-
Notifications
You must be signed in to change notification settings - Fork 45
Run tests on conda build #115
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.
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
Run tests on conda build #115
Conversation
roryyorke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this PR I gather you are not in favour of removing all of the conda recipes?
conda-recipe-apple/meta.yaml
Outdated
| imports: | ||
| - slycot | ||
| commands: | ||
| - conda install nose pytest scipy parameterized --yes --quiet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need nose and pytest ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, isn't there a requires subsection for the test section (e.g., https://github.com/conda-forge/slycot-feedstock/blob/10ef9af7b10ce29ef7ba6e370ea6e0e5a1021b52/recipe/meta.yaml#L38)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need
noseandpytest?
no only pytest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, I was looking for that requires section....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like the conda recipes, as a step up to the feedstock. I use them both on osx and Linux.
conda-recipe-mkl/meta.yaml
Outdated
| - slycot | ||
| commands: | ||
| - conda install nose pytest scipy parameterized --yes --quiet | ||
| - python -c "from slycot import test; test()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you install pytest, why not run pytest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Will convert to pytest and a "Requires:" section. Thanks for the suggestions.
|
Travis now runs the tests twice for the conda jobs: First during conda build then the regular test runs with coverage. No big deal, just will take a little longer. |
| imports: | ||
| - slycot | ||
| commands: | ||
| - pytest --pyargs slycot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - pytest --pyargs slycot | |
| - pytest |
Should be taken care of by #110
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slycot/conda-recipe-apple/meta.yaml
Line 10 in 1bf49c1
| string: py{{ environ.get('PY_VER').replace('.', '') }}{{ environ.get('GIT_DESCRIBE_HASH', '') }}_mkl_{{ environ.get('GIT_DESCRIBE_NUMBER', 0) }} |
I cannot use the "suggest change" feature for this one, but the _mkl_ infix needs to be changed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test does not seem to run from the source folder, conda arranges for a temporary folder for the test. pytest needs the --pyargs slycot here. As I understand it from the documentation, this effectively loads and searches the newly built slycot as installed in the test environment and runs its tests.
This is also a nice way to ensure that an installed slycot is still intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I also implemented the _mkl_ -> _obl_ change
Just now saw your comment in work on readme.rst. I chose to now explicitly put openblas in there (my machine did that automatically?), and since there apparently also exists an optimized apple blas somewhere, I think we needs to reserve _apple_ for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re pytest: Sounds fair.
Why back to openblas and not apple system provided? Could this be merged with the openblas recipe dir again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I now got it to build & tests worked with Apple/Accelerate. Had to force the blas vendor for this with -DBLA_VENDOR=Apple.
Adds running the tests to the conda test section.
Forces the correct (conda) numpy include path.