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

Fix dependencies in requirements-dev.txt #5070

Merged
merged 5 commits into from
Oct 14, 2021

Conversation

Sayam753
Copy link
Member

The requirements-dev were updated in PR - #4932. After the merge, RTD could not build docs because of missing dependencies. So, this PR reverts the changes in requirements-dev.txt.

I am not sure of the differences between environment-dev-py37.yml and environment-test-py37.yml. But sphinx_design and sphinx-notfound-page are not present in environment-test* yml files. So, is there a way to generate these yml files from requirements-dev.txt?

@Sayam753
Copy link
Member Author

Sayam753 commented Oct 12, 2021

Turns out that the requirements-dev.txt is generated from conda env files in the pre-commit.

@codecov
Copy link

codecov bot commented Oct 12, 2021

Codecov Report

Merging #5070 (0698b06) into main (65abc6f) will increase coverage by 17.31%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #5070       +/-   ##
===========================================
+ Coverage   61.11%   78.43%   +17.31%     
===========================================
  Files         130      130               
  Lines       24461    24461               
===========================================
+ Hits        14950    19185     +4235     
+ Misses       9511     5276     -4235     
Impacted Files Coverage Δ
pymc/step_methods/metropolis.py 83.48% <0.00%> (+0.44%) ⬆️
pymc/distributions/distribution.py 94.62% <0.00%> (+0.53%) ⬆️
pymc/sampling.py 87.62% <0.00%> (+0.72%) ⬆️
pymc/model.py 84.14% <0.00%> (+1.21%) ⬆️
pymc/tuning/starting.py 83.19% <0.00%> (+1.68%) ⬆️
pymc/backends/report.py 91.60% <0.00%> (+2.09%) ⬆️
pymc/distributions/continuous.py 95.93% <0.00%> (+2.50%) ⬆️
pymc/tests/test_distributions.py 96.34% <0.00%> (+4.31%) ⬆️
pymc/tests/helpers.py 61.90% <0.00%> (+4.76%) ⬆️
pymc/util.py 74.35% <0.00%> (+7.69%) ⬆️
... and 43 more

@OriolAbril
Copy link
Member

(we should probably end up moving that to an issue and not block this PR, but I wanted to start the
discussion here)

I want to take this opportunity and rethink all these conda environments we have. I understand they are helpful and convenient to create our dev environments and they are also used for ci. However, at least in my case, the extra work and burden on updating 4 (8 now) nearly equivalent files with a pre-commit check that seems to only partially work (i.e. pandas has a lower bound in 3.7 and 3.8 but not in 3.9 nor in requirements-dev.txt) is clearly larger than the benefit I get. Moreover, issues with these files, installation and pre-commit blocked #5060. #5062 was only needed because of #5060 being blocked.

I think there are several issues at hand regarding all this.

  • No single source of truth. From what I understand, the pre-commit script somehow takes all conda env files and from them generates the pip file. However, it seems like conda files don't need to be equivalent between them (see for example from add 404 page to docs #5057 how nbsphinx was on some conda files but not all of them, as well as several versions not matching between files).
  • Overly bloated and non-specific dependency files. We have 2/3 (still not completely understand the new test file but it seems like a step in the right direction) types of requirements files, library requirements (user facing), dev and test ones. But we use those files to do much more than 3 types of things. We generate local envs for devs, we set up CI envs in github acions for testing, we set up the env on rtd to build documentation... So here are some things that we are doing and I think we should avoid as they also make our ci more brittle and prone to conda finding version conflicts:
    • We are installing sphinx and sphinx extensions everywhere, even on github actions
    • We are installing pytest on rtd
    • We are installing libraries like watermark or sphinx-autobuild also everywhere, but these are only used locally! watermark is only needed when writing/running example notebooks but not to build the docs that contain such notebooks. sphinx-autobuild is only needed to get an auto-updating preview of the docs locally.
  • Most of us (or maybe even all of us) have no idea about what some of these dependencies are for. I for example have no idea what cachetools is for. I think that this is both issue and a consequence of the previous two, but I think it's important to put that on the table, because in my opinion, unless we make an effort in explaining all that in the developer guide and keep it up to date it will only get worse.

Ideas for possible solutions:

  • Being more strict about CI. I have the feeling that some PRs are blocked when only pre-commit fails when others are green-lighted with failing tests and/or rtd job. I haven't written much tests myself here and I imagine testing distributions is inherently hard, but I think we need some way of getting rid of flaky tests or grouping them in a "potentially flaky" job, any idea here welcome. I think that having flaky tests makes ourselves open to the possibility of "maybe this is a flaky test too, I don't recall changing anything that should affect this test" or similar reasonings
  • Being more strict about reviews. In a similar manner, I have a feeling that some PRs are merged without approving reviews or with approving reviews even though there were still open questions in the PR comments.
  • Split requirements into task related chunks. This by design eliminates the 1 line to dev environment/single file to serve all, so we'll need to install 2-4 files, I'm also not sure how that would fit with all the pre-commit scripts but I'm sure we'll figure it out, maybe even write a helper script to keep creating dev envs a single line operation. Here are some ideas:
    • requirements.txt -> requirements like we have now for pymc
    • requirements-optional.txt -> optional requirements for pymc, we could also update setup.py so that running pip install pymc[all] installs both requiremetns and requirements-optional
    • requirements-test.txt -> requirements for testing
    • requirements-docs.txt -> requirements for building the docs
    • requirements-write.txt -> requirements for writing example notebooks, see https://github.com/pymc-devs/pymc-examples/blob/main/requirements-write.txt
    • others?
  • Documenting everything. Explain on the developer guide/contributing guide when should each file be used plus maybe also extra recommendations for specific local env only tools (doc related examples of that would be sphinx-autobuild or sphobjinv libraries)

@Sayam753
Copy link
Member Author

+1 on revisiting how we bundle requirements.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I have removed unnecessary dependencies from the ci only files to simplify conda's job and hopefully speed up a bit the env creation process

I have also removed recommonmark and nbsphinx from everywhere they appeared. We don't use any of the two anymore, nor we have done since #4759. Please don't add them again, this has to be the 3rd or 4th PR where I have removed them from the requirements.

@OriolAbril OriolAbril merged commit 597782b into pymc-devs:main Oct 14, 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.

2 participants