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

Organize Python dependencies in separate requirements files #2834

Merged
merged 10 commits into from
Nov 22, 2023

Conversation

nicolossus
Copy link
Member

@nicolossus nicolossus commented Jun 21, 2023

This PR migrates the Python dependencies that were listed in environment.yml to separate requirements files following the already used distinction, that is, requirements for PyNEST, documentation, testing and NEST Server.

Previously, most Python dependencies were installed with conda. However, besides for large Python packages like NumPy, a conda build of a package is usually not continually maintained. An example of this is Sphinx; the version now available with pip is 7.0.1 and 5.0.2 with conda.

Besides ensuring more up-to-date Python dependencies, this PR makes it easier to only use pip to install required Python packages for those that don't want to use conda (see requirements.txt).

The complexity of maintaining our dependencies should not increase with this PR since the environment.yml uses the requirements files to create the conda environment.

Changes to conda environment.yml

  • Added graphviz (needed for pydot)

Conda now installs the non-Python requirements of the software stack (with a couple of exceptions) and pip installs the Python requirements.

Changes to PyNEST requirements

  • Added black
  • Added csa
  • Added pre-commit
  • Removed ipython (automatically installed by notebook)

Note that packages essential to PyNEST that were listed under other sections now have been moved to requirements_pynest.txt.

Changes to documentation requirements

  • Removed path.py. path.py has been renamed to path. However, it looks like all code now use pathlib from the standard library instead.
  • Removed mock. Could not find it used anywhere. If still needed, mock is now part of the standard library, available as unittest.mock in Python 3.3 onwards.
  • Removed recommonmark. Seems to not be used. The package was archieved in 2022. MyST can be used as replacement.

Note that there already exists a doc/requirements.txt where the versions are pinned for Read the Docs. I opted to also let this file exist for the time being, since I am unfamiliar with the reason behind pinning the versions. However, the requirements_doc.txt introduced by this PR should be able to replace doc/requirements.txt.

Changes to testsuite requirements

None.

Changes to NEST Server requirements

All packages retained except for werkzeug which is automatically installed by Flask. The names of a few packages are changed so they correspond with the official spelling: flask -> Flask, flask_cors -> flask-cors, restrictedpython -> RestrictedPython.

@nicolossus nicolossus added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Jun 21, 2023
@jougs
Copy link
Contributor

jougs commented Jul 3, 2023

I wholeheartedly agree with this change from a conceptual point of view. Having the dependencies defined in individual files and using those from whereever seems like a very good idea.

That said, I'm not a user of Conda nor of any other Python environment tools, so I am definitely not the one to review this and suggest to request a review from someone else.

@jougs jougs requested review from Helveg and removed request for jougs July 3, 2023 10:19
Copy link
Contributor

@Helveg Helveg left a comment

Choose a reason for hiding this comment

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

👍 Gives more fine grained control, and allows the requirements to be installed independently of the package manager you decide to use. One remark though: won't the conda users be annoyed to find out most of the environment was installed by pip? Aren't conda users usually recommended not to use pip inside of conda?

https://conda.io/projects/conda/en/latest/user-guide/tasks/manage-pkgs.html#installing-non-conda-packages
https://www.anaconda.com/blog/using-pip-in-a-conda-environment (Best Practices Checklist)

environment.yml Outdated Show resolved Hide resolved
@heplesser
Copy link
Contributor

@terhorstd Would you be able to review this quickly? #2881 touches the files changed here, and so would my solution to #2868.

@heplesser
Copy link
Contributor

@nicolossus Could you resolve the conflict?

environment.yml Outdated Show resolved Hide resolved
Conflicts:
	environment.yml
@heplesser
Copy link
Contributor

@terhorstd Ping!

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

Seems like this is currently the best way to go. (Even though this mechanism has various drawbacks.)

@Helveg
Copy link
Contributor

Helveg commented Nov 19, 2023

@terhorstd if they're not already listed, could you summarize the drawbacks? Always nice for future reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants