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.

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

Update requirements.txt to include missing packages #495

Closed
wants to merge 2 commits into from

Conversation

darothen
Copy link

When installing via the instructions pip install git+https://github.com/atmos-cloud-sim-uj/PySDM.git in the README from a fresh conda environment w/ Python=3.8, pystrict was not installed so neither the tests nor the demo code could run.

When installing via the instructions `pip install git+https://github.com/atmos-cloud-sim-uj/PySDM.git` in the README from a fresh conda environment w/ Python=3.8, `pystrict` was not installed so neither the tests nor the demo code could run.
@darothen darothen changed the title Update requirements.txt to include pystrict Update requirements.txt to include missing packages Apr 27, 2021
@darothen
Copy link
Author

Turns out exdown also was not automatically installed; added.

@codecov-commenter
Copy link

Codecov Report

Merging #495 (14e9c52) into master (87eb462) will not change coverage.
The diff coverage is n/a.

❗ Current head 14e9c52 differs from pull request most recent head 2f07d32. Consider uploading reports for the commit 2f07d32 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #495   +/-   ##
=======================================
  Coverage   80.05%   80.05%           
=======================================
  Files         145      145           
  Lines        4291     4291           
=======================================
  Hits         3435     3435           
  Misses        856      856           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87eb462...2f07d32. Read the comment docs.

@slayoo
Copy link
Member

slayoo commented Apr 27, 2021

thanks! (and thank you for the JOSS review!)
I've just removed the pystrict usage from formulae.py - this was the only place the package was used outside of examples, and there seem to be no need for it here: 7130ba0

@slayoo
Copy link
Member

slayoo commented Apr 27, 2021

Turns out exdown also was not automatically installed; added.

exdown was already here in the requirements, yet I feel that the issue is that we have the requirements split into those listed in setup.py and in requirements.txt. When doing pip install the list from setup.py is used.

Our intention was to use setup.py for the requirements of the code itself, while the requirements.txt file includes packages needed to run tests. The requirements.txt file is used in the CI scripts. Feedback very welcome on how to better address it!

@darothen
Copy link
Author

Aha, that makes sense @slayoo. I'm not sure I have a good recommendation off the top of my head other than to keep them synced up manually. Some folks will read in their requirements.txt file in the setup method but that's also kind of finicky. For the sake of the JOSS review, as long as the demo code works out-of-the-box when a user does the recommended installation technique, that's more than fine - the goal is accessibility and ease of use more than anything else, as I understand it.

@slayoo
Copy link
Member

slayoo commented Apr 28, 2021

One thing that should certainly be improved here is to catch the original problem pointed out by @darothen on CI.

Currently, with Github actions, what we do is as follows:

  • install with setup.py (including dependencies: ThrustRTC, CURandRTC, numba, numpy, Pint, chempy, scipy)
  • install test-time requirements from requirements.txt(matplotlib, ghapi, pytest, exdown, PySDM-examples)
  • run the tests with pytest

The reason to have requirements split across setup.pyand requirements.txt is to cater, e.g., to the users who intend to use PySDM from Matlab or Julia and will not use the examples - real-world case: a Matlab-written parcel model based on PySDM.

The situation caught by @darothen is as follows: we've introduced a new dependency into PySDM code (pystrict) without adding it to the requirements listed in setup.py. However, it was already a dependency of PySDM-examples. So, using PySDM was possible because of installing PySDM-examples, while doing pip install PySDM would actually not give the user a functional instance of PySDM.

So, let's add some hello-world-executing (perhaps even just import) step to CI which will check if using PySDM is possible right after installing it, and before installing other packages.

@slayoo
Copy link
Member

slayoo commented Apr 28, 2021

here is a PR addressing the above: #496
Let me then close this one.
Thank you @darothen !

@slayoo slayoo closed this Apr 28, 2021
@slayoo slayoo mentioned this pull request Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants