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

updated warning message about outdated list of dependencies #4719

Merged
merged 14 commits into from
Jan 3, 2025

Conversation

RohitP2005
Copy link
Contributor

Description

This PR updates the warning message for required dependencies in the index.rst file. The previous message was unclear, and this change provides more specific instructions for users. No functional changes were made to the code, only to the documentation for better clarity.

Fixes #<issue_number> (if applicable)

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Optimization (back-end change that speeds up the code)

Key checklist:

  • [x No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python -m pytest (or $ nox -s tests)
  • The documentation builds: $ python -m pytest --doctest-plus src (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ nox -s quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@Saransh-cpp

@RohitP2005
Copy link
Contributor Author

@Saransh-cpp i guess this includes the changes requested by you.

@arjxn-py
Copy link
Member

i guess this includes the changes requested by you.

Hi, a small tip that It is nice to mention in the PR description where this change was originally requested or something like related or follow-up to #xyz :)

Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

image

Thanks for opening this PR :)
I don't think this is rendering as we want.

@@ -47,7 +47,7 @@ Dependencies

.. warning::

The list of dependencies below might be outdated. Please refer to the ``pyproject.toml`` file to find all dependencies.
The list of dependencies below might be outdated. Please use tools like [``pipdeptree``](https://github.com/tox-dev/pipdeptree) to find the latest dependencies of ``pybamm``
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The list of dependencies below might be outdated. Please use tools like [``pipdeptree``](https://github.com/tox-dev/pipdeptree) to find the latest dependencies of ``pybamm``
The list of dependencies below might be outdated. Please use tools like [`pipdeptree`](https://github.com/tox-dev/pipdeptree) to find the latest dependencies of ``pybamm``

Maybe this?

Copy link
Member

Choose a reason for hiding this comment

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

The PyData Sphinx Theme already adds icons for popular websites including GitHub (as noted in the above screenshot), so we should be fine without rendering Markdown (because this is a docs-only page).

But, I don't recommend using tools like pipdeptree in the first place, since as I mentioned in #4706 (comment), it cannot handle optional dependencies at the moment: tox-dev/pipdeptree#107. Neither can uv pip tree: astral-sh/uv#4710. It is a metadata problem and not an implementation problem: pypa/packaging-problems#215

So, if we are to improve this warning message, we need something better here.

@@ -47,7 +47,7 @@ Dependencies

.. warning::

The list of dependencies below might be outdated. Please refer to the ``pyproject.toml`` file to find all dependencies.
The list of dependencies below might be outdated. Please use tools like [``pipdeptree``](https://github.com/tox-dev/pipdeptree) to find the latest dependencies of ``pybamm``
Copy link
Member

Choose a reason for hiding this comment

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

The PyData Sphinx Theme already adds icons for popular websites including GitHub (as noted in the above screenshot), so we should be fine without rendering Markdown (because this is a docs-only page).

But, I don't recommend using tools like pipdeptree in the first place, since as I mentioned in #4706 (comment), it cannot handle optional dependencies at the moment: tox-dev/pipdeptree#107. Neither can uv pip tree: astral-sh/uv#4710. It is a metadata problem and not an implementation problem: pypa/packaging-problems#215

So, if we are to improve this warning message, we need something better here.

@RohitP2005
Copy link
Contributor Author

I apologies for the confusion caused here .
The pr was requested by @Saransh-cpp at #4706 (comment) despite @agriyakhetarpal suggesting it was not necessary.
I would close the PR if this change is uncessary

@agriyakhetarpal
Copy link
Member

I'll be happy to approve if we have something better here. :) We could just say something like,

"Users are advised to manually check the list of dependencies to find out supported versions".

Or, I like SciPy's idea – where they say, "whatever recent versions work": https://docs.scipy.org/doc/scipy/dev/toolchain.html#building-the-documentation, and only enforce or update the lower bound for a dependency only in the case there is a breakage with versions lower than it.

@RohitP2005
Copy link
Contributor Author

image
image

I think this might be good way to put them

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Thanks, @RohitP2005! Looks good to me. We should also update the versions as well, while we are at it:

docs/source/user_guide/installation/index.rst Outdated Show resolved Hide resolved
docs/source/user_guide/installation/index.rst Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member

@Saransh-cpp, does this fit your previous suggestion better?

@RohitP2005
Copy link
Contributor Author

Thanks, @RohitP2005! Looks good to me. We should also update the versions as well, while we are at it:

Happy to help anytime

Copy link

codecov bot commented Dec 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.25%. Comparing base (a7253b8) to head (05136e4).
Report is 19 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4719      +/-   ##
===========================================
+ Coverage    99.22%   99.25%   +0.02%     
===========================================
  Files          303      303              
  Lines        23070    23230     +160     
===========================================
+ Hits         22891    23056     +165     
+ Misses         179      174       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agriyakhetarpal agriyakhetarpal changed the title updated warning message updated warning message about outdated list of dependencies Dec 31, 2024
@RohitP2005
Copy link
Contributor Author

@agriyakhetarpal do u think, are there anything else that can be improved here

@agriyakhetarpal
Copy link
Member

No more improvements; I'll suggest @Saransh-cpp to review if he finds anything, otherwise, I shall merge.

arjxn-py
arjxn-py previously approved these changes Jan 1, 2025
Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Thanks @RohitP2005 looks good :)

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

The pr was requested by @Saransh-cpp at #4706 (comment) despite @agriyakhetarpal suggesting it was not necessary.

I agreed to add a link to the file instead of using another tool in my previous review.

docs/source/user_guide/installation/index.rst Outdated Show resolved Hide resolved
@@ -49,7 +49,7 @@ PyBaMM requires the following dependencies:

.. warning::

The list of dependencies below might be outdated. Please refer to the ``pyproject.toml`` file to find all dependencies.
The list of dependencies below might be outdated. Users are advised to manually check the ``pyproject.toml`` file to find out supported versions.
Copy link
Member

Choose a reason for hiding this comment

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

A link would be great :)

@RohitP2005 RohitP2005 dismissed stale reviews from arjxn-py and agriyakhetarpal via 1c7cc70 January 3, 2025 03:39
Saransh-cpp
Saransh-cpp previously approved these changes Jan 3, 2025
docs/source/user_guide/installation/index.rst Outdated Show resolved Hide resolved
@Saransh-cpp Saransh-cpp enabled auto-merge (squash) January 3, 2025 11:20
@Saransh-cpp Saransh-cpp merged commit f79cd26 into pybamm-team:develop Jan 3, 2025
24 checks passed
@RohitP2005
Copy link
Contributor Author

Thanks @Saransh-cpp

@RohitP2005 RohitP2005 deleted the warning-issue branch January 4, 2025 07:17
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.

5 participants