Skip to content

Create Optional Dependencies#3044

Merged
Saransh-cpp merged 34 commits intopybamm-team:developfrom
arjxn-py:make-deps-optional
Jul 6, 2023
Merged

Create Optional Dependencies#3044
Saransh-cpp merged 34 commits intopybamm-team:developfrom
arjxn-py:make-deps-optional

Conversation

@arjxn-py
Copy link
Copy Markdown
Member

@arjxn-py arjxn-py commented Jun 16, 2023

Description

The goal of this PR is to make most of the dependencies optional & split them in specific group

Fixes #3035

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)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (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 run-tests.py --all
  • The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

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

@arjxn-py
Copy link
Copy Markdown
Member Author

Related to #3035

@arjxn-py arjxn-py marked this pull request as draft June 16, 2023 13:38
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (c494873) 99.71% compared to head (61f79cf) 99.71%.

❗ Current head 61f79cf differs from pull request most recent head 63524b4. Consider uploading reports for the commit 63524b4 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3044      +/-   ##
===========================================
- Coverage    99.71%   99.71%   -0.01%     
===========================================
  Files          248      248              
  Lines        18749    18747       -2     
===========================================
- Hits         18695    18693       -2     
  Misses          54       54              
Impacted Files Coverage Δ
pybamm/input/parameters/lithium_ion/Prada2013.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@arjxn-py arjxn-py marked this pull request as ready for review June 17, 2023 14:47
@Saransh-cpp Saransh-cpp self-requested a review June 17, 2023 23:04
@arjxn-py arjxn-py marked this pull request as draft June 19, 2023 16:13
@arjxn-py arjxn-py marked this pull request as ready for review June 20, 2023 08:16
@arjxn-py
Copy link
Copy Markdown
Member Author

Read the docs build failed, trying adding tqdm in docs/requirements.txt

@arjxn-py
Copy link
Copy Markdown
Member Author

@Saransh-cpp maybe we could group pybtex , sympy , bpx & tqdm together?

Copy link
Copy Markdown
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.

Thanks, @arjxn-py! Looks good, few comments -

Comment thread setup.py Outdated
Comment thread setup.py Outdated
Comment thread setup.py Outdated
Comment thread setup.py Outdated
Copy link
Copy Markdown
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

Before we merge this it's important to document what the different options do in the installation docs, like in pandas https://pandas.pydata.org/docs/getting_started/install.html#optional-dependencies

@arjxn-py
Copy link
Copy Markdown
Member Author

Thanks for the review both, i'm looking into the suggested things.

@arjxn-py arjxn-py force-pushed the make-deps-optional branch from 0d3a0a1 to e1b246b Compare July 1, 2023 11:53
@arjxn-py arjxn-py requested a review from brosaplanella July 3, 2023 07:52
@arjxn-py arjxn-py force-pushed the make-deps-optional branch 3 times, most recently from f4c3179 to f091e27 Compare July 3, 2023 13:32
Copy link
Copy Markdown
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.

Great work, thanks @arjxn-py! Looks good overall, but a few minor comments below 🙂

Comment thread noxfile.py
Comment thread noxfile.py Outdated
Comment thread noxfile.py
Comment thread docs/source/user_guide/installation/index.rst
Comment thread CHANGELOG.md Outdated
@arjxn-py
Copy link
Copy Markdown
Member Author

arjxn-py commented Jul 3, 2023

@brosaplanella Probably it would be better to have a separate PR for jupyter notebooks as pip install pybamm[all] is currently not supported. Although tests are passing but when running notebooks it is giving warning & then installing pybamm without any extras, so if I open a PR for jupyter notebooks & merge it after a patch release so that we have pybamm[all] distributed on pip to install without any issue.

@brosaplanella
Copy link
Copy Markdown
Member

@brosaplanella Probably it would be better to have a separate PR for jupyter notebooks as pip install pybamm[all] is currently not supported. Although tests are passing but when running notebooks it is giving warning & then installing pybamm without any extras, so if I open a PR for jupyter notebooks & merge it after a patch release so that we have pybamm[all] distributed on pip to install without any issue.

I don't think we need to do a patch release for this. In theory the links to Colab should use the notebooks in main, so it should not be an issue, but probably it is safer to have a separate PR that we can merge in a month or so, just before the feature freeze for 23.9. What are your thoughts @tinosulzer and @Saransh-cpp?

@arjxn-py
Copy link
Copy Markdown
Member Author

arjxn-py commented Jul 4, 2023

I don't think we need to do a patch release for this. In theory the links to Colab should use the notebooks in main, so it should not be an issue, but probably it is safer to have a separate PR that we can merge in a month or so, just before the feature freeze for 23.9. What are your thoughts @tinosulzer and @Saransh-cpp?

Got it, +1

@arjxn-py arjxn-py requested a review from Saransh-cpp July 4, 2023 13:05
Comment thread CHANGELOG.md Outdated
@agriyakhetarpal
Copy link
Copy Markdown
Member

A note that the Open in Colab badge in the README is currently pointing to the develop branch, we should change it to point to the main branch: https://colab.research.google.com/github/pybamm-team/PyBaMM/blob/main/

@arjxn-py arjxn-py force-pushed the make-deps-optional branch from d596696 to 50f20cc Compare July 4, 2023 14:51
@arjxn-py arjxn-py requested a review from Saransh-cpp July 4, 2023 15:00
Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md Outdated
Co-authored-by: Saransh Chopra <saransh0701@gmail.com>
Copy link
Copy Markdown
Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@Saransh-cpp Saransh-cpp force-pushed the make-deps-optional branch from 61f79cf to 63524b4 Compare July 5, 2023 12:53
Copy link
Copy Markdown
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

thanks, looks good

@Saransh-cpp Saransh-cpp merged commit fffe650 into pybamm-team:develop Jul 6, 2023
@arjxn-py arjxn-py deleted the make-deps-optional branch August 4, 2023 13:27
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.

Splitting the required dependencies into optional dependencies

5 participants