Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Increase testing parallelism #3620

Merged
merged 17 commits into from May 25, 2018
Merged

Increase testing parallelism #3620

merged 17 commits into from May 25, 2018

Conversation

jigold
Copy link
Collaborator

@jigold jigold commented May 17, 2018

No description provided.

tpoterba
tpoterba reviewed May 17, 2018
View reviewed changes
build.gradle
environment PYSPARK_SUBMIT_ARGS: '--conf spark.driver.extraClassPath=' + projectDir + '/build/libs/hail-all-spark.jar --conf spark.executor.extraClassPath=' + projectDir + '/build/libs/hail-all-spark.jar pyspark-shell'
environment PYSPARK_PYTHON: 'python3'
environment NOSE_NOLOGCAPTURE: '1'
commandLine 'pytest',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I figured why pytest was faster than nose -- we aren't running pytest with coverage enabled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I always get error messages abt coverage not being available. I think this was on the cloud too. I can double check.

danking
danking reviewed May 18, 2018
View reviewed changes
build.gradle
commandLine 'pytest',
'-n',
parallelism,
'--dist=loadscope',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't find the docs for this option, can you point me to them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Disregard I see that this is an option on pytest-xdist rather than pytest.

danking
danking requested changes May 18, 2018
View reviewed changes
python/hail/tests/test_methods.py Outdated
@@ -909,16 +909,6 @@ def test_pca(self):

_, _, loadings = hl.pca(mt.GT.n_alt_alleles(), k=2, compute_loadings=False)
self.assertEqual(loadings, None)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems bad to completely remove a test?

Copy link
Contributor

Choose a reason for hiding this comment

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

we briefly discussed this yesterday. This test was created as part of our "if you fix a bug, make a test" policy, and fixed a bug caused by PCA doing its own AST/eval stuff. PCA now calls the base expr API, so this test is totally unnecessary. The expr stuff should be tested in its own modules, and PCA should be tested for its own method correctness (#3613), but we shouldn't need to test whether all expressions are valid input for PCA (doing this everywhere would be insane!)

Copy link
Contributor

Choose a reason for hiding this comment

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

also, I don't think this is even a valid test given that we don't guarantee determinism

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok! In the future, let's try to keep these changes as separate PRs with a description as to why we are removing it. I'll approve now! 😀

danking
danking previously approved these changes May 18, 2018
View reviewed changes
python/hail/tests/test_methods.py Outdated
@@ -909,16 +909,6 @@ def test_pca(self):

_, _, loadings = hl.pca(mt.GT.n_alt_alleles(), k=2, compute_loadings=False)
self.assertEqual(loadings, None)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok! In the future, let's try to keep these changes as separate PRs with a description as to why we are removing it. I'll approve now! 😀

tpoterba
tpoterba previously requested changes May 18, 2018
View reviewed changes
python/hail/dev-environment.yml
@@ -17,3 +17,4 @@ dependencies:
- pip:
- parsimonious
- ipykernel
- pytest-xdist
Copy link
Contributor

Choose a reason for hiding this comment

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

we need pytest in the dependencies as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove nose also

@jigold
Copy link
Collaborator Author

jigold commented May 22, 2018

@tpoterba I had everything working locally, but hadn't updated master. Unfortunately, there's a test failure in the new test you added for index_globals and I can't figure out how to fix it. I tried creating the environment separately and the pli field was there. Could you please take a look?

_____________ [doctest] hail.matrixtable.MatrixTable.index_globals _____________
[gw0] darwin -- Python 3.6.1 //anaconda/envs/py36/bin/python
UNEXPECTED EXCEPTION: AttributeError("StructExpression instance has no field, method, or property 'pli'\n    Hint: use 'describe()' to show the names of all data fields.",)
Traceback (most recent call last):

  File "//anaconda/envs/py36/lib/python3.6/doctest.py", line 1330, in __run
    compileflags, 1), test.globs)

  File "<doctest hail.matrixtable.MatrixTable.index_globals[0]>", line 1, in <module>

  File "/Users/jigold/hail/build/tmp/doctest/python/hail/expr/expressions/typed_expressions.py", line 1161, in __getattr__
    raise AttributeError(get_nice_attr_error(self, item))

AttributeError: StructExpression instance has no field, method, or property 'pli'
    Hint: use 'describe()' to show the names of all data fields.

/Users/jigold/hail/build/tmp/doctest/python/hail/matrixtable.py:2197: UnexpectedException
============== 1 failed, 420 passed, 13 skipped in 79.03 seconds ===============

@jigold
Copy link
Collaborator Author

jigold commented May 22, 2018

To test locally, ./gradlew --daemon doctest

@jigold jigold dismissed stale reviews from tpoterba and danking May 22, 2018 22:30

done

tpoterba
tpoterba previously approved these changes May 23, 2018
View reviewed changes
@jigold
Copy link
Collaborator Author

jigold commented May 25, 2018

@tpoterba This is all set now.

@tpoterba tpoterba merged commit 0b7c877 into hail-is:master May 25, 2018
konradjk pushed a commit to konradjk/hail that referenced this pull request Jun 12, 2018
@jigold @konradjk
Increase testing parallelism (hail-is#3620)
864ce65 
* Increase testing parallelism

* rebase & address comments

* doctest improvements

* fix tutorial

* forgot to add files

* fix test

* wip

* wip

* working

* remove errant comment

* forgot to add files

* add back extension

* fix sphinx

* test delete clean

* debug make wd

* fix?

* fix directory cp
jackgoldsmith4 pushed a commit to jackgoldsmith4/hail that referenced this pull request Jun 25, 2018
@jigold @jackgoldsmith4
Increase testing parallelism (hail-is#3620)
1aab569 
* Increase testing parallelism

* rebase & address comments

* doctest improvements

* fix tutorial

* forgot to add files

* fix test

* wip

* wip

* working

* remove errant comment

* forgot to add files

* add back extension

* fix sphinx

* test delete clean

* debug make wd

* fix?

* fix directory cp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpoterba tpoterba tpoterba approved these changes

@danking danking danking left review comments

Assignees

@danking danking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jigold @danking @tpoterba