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

Cleaning Up Project Dependencies #91

Closed
justinvasel opened this issue Feb 2, 2024 · 4 comments · Fixed by #96
Closed

Cleaning Up Project Dependencies #91

justinvasel opened this issue Feb 2, 2024 · 4 comments · Fixed by #96
Labels
enhancement New feature or request help wanted Extra attention is needed question Further information is requested

Comments

@justinvasel
Copy link
Contributor

justinvasel commented Feb 2, 2024

Overview

Project dependencies in both snews_pt and snews_cs could use some TLC. snews_cs depends on snews_pt, so let's handle this one first, and then move on to tidying up the snews_cs side.

Here are some issues I've spotted:

  • Some listed packages are not project dependencies, but dependencies of project dependencies; they don't need to be specified by us.
  • Old, end-of-life versions of Python are still supported.
  • Some outdated packages contain known vulnerabilities and need patching
  • snews_cs depends on snews_pt, which creates some version mis-match issues for dependencies that they share in common; bringing dependencies up-to-date in both packages should address most of these issues.

I've done a quick audit of snews_pt package requirements, and came up with the recommendations below. Hoping to get the group's input on this, particularly whether removing any of the packages I've recommended below will disrupt anyone's workflow.

Here is a list of related issues we could close out with these changes:
SNEWS PT
#78

SNEWS CS
SNEWS2/SNEWS_Coincidence_System#94
SNEWS2/SNEWS_Coincidence_System#91
SNEWS2/SNEWS_Coincidence_System#55
SNEWS2/SNEWS_Coincidence_System#8
SNEWS2/SNEWS_Coincidence_System#5

Please leave any comments/questions/concerns you have about these recommendations in the chat and we can iterate. Once we've had a chance to discuss, I can submit a PR with whatever changes we decided on.

Minimum Supported Versions

Python

CONTEXT

  • snews_pt currently supports Python 3.7 and up.
  • Python 3.7 has already reached end-of-life, and 3.8 will reach EOL before the end of this year.
  • Python 3.8–3.10 currently only receive security fixes; 3.11 & 3.12 are the latest versions and receive bug fixes.
  • Scientific Python's spec on minimum supported versions recommends >=3.11

RECOMMENDATION
snews_pt should drop support for Python versions 3.7 and 3.8, and should consider dropping support for 3.9 and 3.10 if current SNEWS experiments have access to 3.11 or 3.12 installations.

Packages

CONTEXT
The table below lists snews_pt package dependencies based on the contents of requirements.txt and setup.py. Some packages are unnecessary because they are second- or third-order dependencies, and many are out of date. Some packages don't appear to be used by snews_pt at all. However, some may be command-line tools that developers use manually in the course of testing, linting, etc.

Group Package Current Recommended Used in code?
Required attrs >=21.4.0 REMOVE NO
Required click >=8.1.2 ~=8.1 YES
Required docutils ==0.17.1 REMOVE NO
Required fixtures ==3.0.0 REMOVE NO
Required hop-client ==0.8.0 ~=0.8 YES
Required inquirer >=2.8.0 ~=2.8 YES
Required ipython ~=7.32.0 ~=8.7 YES
Required mock ==4.0.3 REMOVE NO
Required myst_parser ==0.16.1 ~=2.0 YES
Required nose ==1.3.7 REMOVE NO
Required numpy >=1.22.3 ~=1.26 YES
Required python-dotenv >=0.19.2 ~=0.19 YES
Required setuptools >=62.1.0 ~=69.0 YES
Required six ==1.14.0 REMOVE NO
Required testrepository ==0.0.20 REMOVE NO
Required testresources ==2.0.1 REMOVE NO
Required testscenarios ==0.5.0 REMOVE NO
Required testtools ==2.5.0 REMOVE NO
Required wheel ==0.34.2 ~=0.42 YES
dev virtualenv ==20.13.0 ~=20.13 YES
dev autopep8 (any) REMOVE NO
dev flake8 (any) REMOVE NO
dev mongomock (any) REMOVE NO
dev pytest >=5.0, <5.4 ~=8.0 YES
dev pytest-console-scripts (any) ~=1.4 UNKNOWN
dev pytest-cov (any) ~=4.1 UNKNOWN
dev pytest-mongodb (any) ~=2.4 UNKNOWN
dev pytest-runner (any) ~=6.0 UNKNOWN
dev twine (any) REMOVE NO
dev schedule (any) REMOVE NO
doc autoapi (any) ~=2.0 YES
doc myst_parser (any) ~=2.0 YES
doc sphinx (any) ~=7.2 YES
doc sphinx-autoapi ==1.8.4 ~=3.0 YES
doc sphinx_rtd_theme ==1.0.0 ~=2.0 YES
doc sphinxcontrib-programoutput ==0.17 ~=0.17 YES

RECOMMENDATION
Upgrade or remove packages based on the recommended column in the table above. In most cases, allow use of the latest minor version, but don't allow use of any major versions that are larger than the listed one.

Implementation

  1. Collect feedback on inclusion/removal of packages listed in the above table
  2. Updated supported python version in setup.py
  3. Update requirements.txt with only the "Required" group packages from above
  4. Update setup.py with "dev" and "doc" group packages from above
  5. Update any relevant documentation
  6. Confirm tests pass and docs build
  7. Merge PR and cut new release
@justinvasel justinvasel added enhancement New feature or request help wanted Extra attention is needed question Further information is requested labels Feb 2, 2024
@sybenzvi
Copy link
Contributor

sybenzvi commented Feb 2, 2024

Looks great @justinvasel. Only comment is that autoapi~=8.1 doesn't exist, so instead we should use autoapi~=2.0.1 or autoapi~=2.0.

@sybenzvi
Copy link
Contributor

sybenzvi commented Feb 2, 2024

Readthedocs is now working again (see here). Note that I created docs/requirements.txt just to test everything; we could copy it to the source root and just use the package-level readthedocs for both documentation and installation.

@KaraMelih
Copy link
Collaborator

I just got back and started looking into documentation issues/PRs. Seems like most of it (if not all) is sorted out. Just in case, I added @sybenzvi as a maintainer on both RTD pages. I could not find @justinvasel if you have an account I can also add you as a maintainer.

@justinvasel
Copy link
Contributor Author

TL;DR: I submitted PR #96 to address this. Looking for folks to test and make sure everything works as expected on their own machines.

Are any experiments actively using this package to report to SNEWS?
If so, we should verify they can support python 3.11 and 3.12 before merging and tagging a new release. If not, let's push it out before they do!

===

Looks great @justinvasel. Only comment is that autoapi~=8.1 doesn't exist, so instead we should use autoapi~=2.0.1 or autoapi~=2.0.

Thanks Segev; I updated the table accordingly.

I just got back and started looking into documentation issues/PRs. Seems like most of it (if not all) is sorted out. Just in case, I added @sybenzvi as a maintainer on both RTD pages. I could not find @justinvasel if you have an account I can also add you as a maintainer.

I created an account there just now, so feel free to add me as a maintainer. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants