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

Test Optional Dependencies #3892

Merged
merged 22 commits into from
Mar 18, 2024

Conversation

lorenzofavaro
Copy link
Contributor

@lorenzofavaro lorenzofavaro commented Mar 13, 2024

Description

This PR adds a test module for optional dependencies just for the core version of PyBaMM.
Important changes made:

  • Remove anytree from test_import_optional_dependency (it's a required dependency, not an optional one).
  • Make pandas a required dependency. It is actually installed along with xarray (currently a required dependency).
    To verify it:
     python3.11 -m venv venv # create new environment
     source venv/bin/activate # access it
     pip install xarray # install xarray
     pip list # show installed packages
    This step avoids finding it among the optional dependencies when we parse them using importlib.metadata('pybamm'). Since the core version of pybamm also installs it, the test to check that it does not contain optional dependencies would fail.
  • Add a separate test module. The reason for this is that the test TestDependencies.test_optional_dependencies as implemented, does not use the function import_optional_dependency* and conceptually does not verify that a unit function is working correctly. It is checking that in the core version of pybamm we are not installing more dependencies than we expect. Moreover, it does not have to be inside a module with filename test_* because it would always be executed in any test session. Please, tell me what you think about this and if you suggest any alternatives.
  • Add a separate job to the test_on_push.yml workflow that launch it with just the core pybamm installation.

*If I'm not wrong, we cannot use import_optional_dependency because distribution packages extracted from importlib.metadata do not necessarily have a 1:1 mapping with import packages, as per documentation:

Important: These are not necessarily equivalent to or correspond 1:1 with the top-level import package names that can be imported inside Python code. One distribution package can contain multiple import packages (and single modules), and one top-level import package may map to multiple distribution packages if it is a namespace package. You can use package_distributions() to get a mapping between them.

Fixes #3865

Type of change

  • 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 (or $ nox -s pre-commit)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

agriyakhetarpal

This comment was marked as outdated.

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (8056d22) to head (4c52b2d).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3892      +/-   ##
===========================================
- Coverage    99.60%   99.60%   -0.01%     
===========================================
  Files          259      259              
  Lines        21273    21268       -5     
===========================================
- Hits         21189    21184       -5     
  Misses          84       84              

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

.github/workflows/run_periodic_tests.yml Outdated Show resolved Hide resolved
.github/workflows/test_on_push.yml Outdated Show resolved Hide resolved
.github/workflows/test_on_push.yml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
@agriyakhetarpal agriyakhetarpal self-requested a review March 14, 2024 17:41
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, @lorenzofavaro – please do not mind the earlier, hasty review that I had put in. This looks good to me barring a few required changes. Some of them, such as those for refining CI can be considered for future PRs, not this one.

pyproject.toml Show resolved Hide resolved
tests/dependencies.py Outdated Show resolved Hide resolved
tests/dependencies.py Outdated Show resolved Hide resolved
tests/dependencies.py Outdated Show resolved Hide resolved
tests/dependencies.py Outdated Show resolved Hide resolved
tests/dependencies.py Outdated Show resolved Hide resolved
tests/unit/test_util.py Show resolved Hide resolved
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.

This looks great @lorenzofavaro, thank you! Approving from my end with one small comment. If no one has any additional comments, we can merge.

pyproject.toml Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member

@all-contributors please add @lorenzofavaro for code, tests

Copy link
Contributor

@agriyakhetarpal

I've put up a pull request to add @lorenzofavaro! 🎉

@agriyakhetarpal agriyakhetarpal mentioned this pull request Mar 18, 2024
8 tasks
@kratman kratman merged commit f3616d7 into pybamm-team:develop Mar 18, 2024
46 checks passed
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this pull request Aug 12, 2024
* Remove anytree test

* Make pandas required

* Add test for optional dependencies

* Add test pybamm import

* Add test job

* Remove comments

* Rename job

* Change exception type

* Filter regex (dev&docs extra)

* Clarify docstrings

* Clarify error message

* Remove test runner

* Reformulate docstrings

* Update changelog and documentation

* Remove test module

* Remove workflows

* Refactor tests

* Set package dynamically

* Fix comment

* Set conditional dependency

---------

Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
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.

Mock all the optional dependencies to testing for better coverage
3 participants