Skip to content

Disable io.ascii multiprocessing on Windows for now - #2969

Merged
embray merged 5 commits into
astropy:masterfrom
mdmueller:disable-multiprocessing-windows
Oct 1, 2014
Merged

Disable io.ascii multiprocessing on Windows for now#2969
embray merged 5 commits into
astropy:masterfrom
mdmueller:disable-multiprocessing-windows

Conversation

@mdmueller

@mdmueller mdmueller commented Sep 22, 2014

Copy link
Copy Markdown
Contributor

As @embray noted in #2952, some io.ascii tests fail on Windows because there is a problem with Windows multiprocessing. This PR temporarily disables multiprocessing on Windows (and contains a minor segfault fix).

@mdmueller mdmueller mentioned this pull request Sep 22, 2014
Closed
embray
embray reviewed Sep 22, 2014
View reviewed changes
Comment thread astropy/io/ascii/cparser.pyx Outdated

@embray embray Sep 22, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my comment in #2952 I think this should outright raise an exception--right now it just doesn't seem to work at all that I can tell :(

@taldcroft

taldcroft commented Sep 22, 2014

Copy link
Copy Markdown
Member

Per the comment from @embray in #2952, if parallel is consistently causing a hang on his installation then we should entirely disable parallel on windows by raising a NotImplemented exception.

@mdmueller

mdmueller commented Sep 22, 2014

Copy link
Copy Markdown
Contributor Author

This latest commit changes from warning to raising a NotImplementedError. I also restructured the tests a bit in order to xfail properly, which makes more sense in retrospect (there had previously been a parameter fail_parallel instead of a more explicit xfail).

@mdmueller
mdmueller force-pushed the disable-multiprocessing-windows branch from f00b110 to e867646 Compare September 22, 2014 22:02
@embray

embray commented Sep 22, 2014

Copy link
Copy Markdown
Member

👍 Thanks!

taldcroft
taldcroft reviewed Sep 23, 2014
View reviewed changes
Comment thread astropy/io/ascii/tests/test_c_reader.py

@taldcroft taldcroft Sep 23, 2014

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you missed putting in parallel=parellel in the read_basic() call below.

@taldcroft

taldcroft commented Sep 23, 2014

Copy link
Copy Markdown
Member

This generally looks OK to me.

As noted it seems like there are some instances where test functions were parameterized with parallel but then the parallel value wasn't actually used. So @AmrAS1 if you can just scan through the diffs and make sure that any omissions are intentional and make sense.

@mdmueller
mdmueller force-pushed the disable-multiprocessing-windows branch from 126ff71 to df18d7e Compare September 23, 2014 21:46
@mdmueller

mdmueller commented Sep 23, 2014

Copy link
Copy Markdown
Contributor Author

@taldcroft - Thanks for the catch (I just did a search/replace with sed, should've checked that). Just looked over the diffs and I think this should be good.

@mdmueller
mdmueller force-pushed the disable-multiprocessing-windows branch from df18d7e to e1674d6 Compare September 24, 2014 00:19
@embray embray added this to the v1.0.0 milestone Oct 1, 2014
@embray embray added Affects-dev PRs and issues that do not impact an existing Astropy release Windows testing io.ascii labels Oct 1, 2014
embray added a commit that referenced this pull request Oct 1, 2014
@embray
Merge pull request #2969 from amras1/disable-multiprocessing-windows
51711ea 
Disable io.ascii multiprocessing on Windows for now
@embray
embray merged commit 51711ea into astropy:master Oct 1, 2014
@taldcroft taldcroft mentioned this pull request Oct 6, 2014
Closed
@mdmueller
mdmueller deleted the disable-multiprocessing-windows branch October 6, 2014 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release io.ascii testing Windows

Projects

None yet

Milestone

v1.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@mdmueller @taldcroft @embray