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

Bring back dependabot #1864

Merged
merged 12 commits into from
Jan 18, 2023

Conversation

StanczakDominik
Copy link
Member

This closes #1861, where the motivation is also laid out. The setup follows https://www.b-list.org/weblog/2022/may/13/boring-python-dependencies/, https://iscinumpy.dev/post/bound-version-constraints/ and dependabot/dependabot-core#3290.

To quote from that last link there:

as long as dependabot recommends lockfiles, and Python doesn’t have a standard lockfile format, it makes sense to say “we recommend using poetry.lock, requirements.txt AS LOCKFILES”, while of course the recommended format for Python package metadata remains PEP 622.

The lockfile concept is the answer to all my doubts from #1861. Essentially, I was right upper pinning dependencies is problematic; but it is not the only way to ensure tests are ran with a particular set of dependencies. Instead, tools (classically poetry, more recently pip-compile) can inspect requirements, e.g. lower and upper bounds on package versions, and create a lockfile, which is basically "at this time, the most recent set of packages that solves all of these constraints". A single point in dependency space right at the bound of the allowed region.

Essentially, the process goes like:

  • specify dependencies with lower bounds and upper bounds in pyproject.toml
  • use pip-compile --resolver=backtracking --all-extras from pip-tools to generate a requirements.txt file that pins everything to specific versions, here's an excerpt:
#
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
#    pip-compile --all-extras --resolver=backtracking
#
alabaster==0.7.12
    # via sphinx
anyio==3.6.2
    # via jupyter-server
argon2-cffi==21.3.0
    # via jupyter-server
argon2-cffi-bindings==21.2.0
    # via argon2-cffi
asteval==0.9.28
    # via lmfit
astor==0.8.1
    # via flake8-simplify
astropy==5.2
    # via plasmapy (setup.py)
  • run our 3.10 test suite off of dependencies specified in that file
  • enable dependabot to keep checking for updates in that requirements.txt file.

This might need a "merge first, adjust later" approach for the actual dependabot part.

@StanczakDominik StanczakDominik requested a review from a team as a code owner January 3, 2023 16:12
@StanczakDominik StanczakDominik requested review from namurphy and removed request for a team January 3, 2023 16:12
@github-actions
Copy link

github-actions bot commented Jan 3, 2023

Thank you for contributing to PlasmaPy! The project's future depends deeply on contributors like you, so we deeply appreciate it! 🌱 The following checklist will be used by the code reviewer to help guide the code review process.

  • Overall
    • Does the PR do what it intends to do?
    • Except for very minor changes, is a changelog entry included and consistent with the changelog guide?
    • Are the continuous integration checks passing? (Most linter problems can be automagically fixed by commenting on this PR with pre-commit.ci autofix.)
  • Code
    • Is new/updated code understandable and consistent with the coding guide?
    • Are there ways to greatly simplify the implementation?
    • Are there any large functions that should be split up into shorter functions?
  • Tests
    • Are tests added/updated as required, and consistent with the testing guide?
    • Are the tests understandable?
    • Do the tests cover all important cases?
  • Docs
    • Are docs added/updated as required, and consistent with the doc guide?
    • Are the docs understandable?
    • Do the docs show up correctly in the preview, including Jupyter notebooks?

Comment on lines +5 to +9
# Maintain dependencies for GitHub Actions
- package-ecosystem: github-actions
directory: /
schedule:
interval: weekly
Copy link
Member Author

Choose a reason for hiding this comment

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

Figured we might as well add these, too.

Copy link
Member

Choose a reason for hiding this comment

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

Yay for automation!

@codecov
Copy link

codecov bot commented Jan 3, 2023

Codecov Report

Base: 98.31% // Head: 98.29% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (01aa7bf) compared to base (5e542ce).
Patch has no changes to coverable lines.

❗ Current head 01aa7bf differs from pull request most recent head cb69cec. Consider uploading reports for the commit cb69cec to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
- Coverage   98.31%   98.29%   -0.02%     
==========================================
  Files          95       95              
  Lines        8404     8409       +5     
==========================================
+ Hits         8262     8266       +4     
- Misses        142      143       +1     
Impacted Files Coverage Δ
plasmapy/particles/ionization_state.py 94.62% <0.00%> (-0.30%) ⬇️
plasmapy/utils/decorators/validators.py 100.00% <0.00%> (ø)
plasmapy/particles/_special_particles.py 100.00% <0.00%> (ø)
plasmapy/particles/particle_collections.py 100.00% <0.00%> (ø)
plasmapy/analysis/swept_langmuir/helpers.py 100.00% <0.00%> (ø)
...cs/charged_particle_radiography/detector_stacks.py 100.00% <0.00%> (ø)
...rged_particle_radiography/synthetic_radiography.py 99.75% <0.00%> (+<0.01%) ⬆️
plasmapy/particles/particle_class.py 98.56% <0.00%> (+<0.01%) ⬆️
plasmapy/particles/decorators.py 99.41% <0.00%> (+<0.01%) ⬆️
plasmapy/utils/_units_helpers.py 96.15% <0.00%> (+0.15%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@StanczakDominik
Copy link
Member Author

The pre-commit.ci failure seems unrelated. I think we can remove it from required temporarily.

tox.ini Outdated Show resolved Hide resolved
.github/dependabot.yml Outdated Show resolved Hide resolved
@namurphy
Copy link
Member

namurphy commented Jan 3, 2023

Should we take this approach, then we might want to update the list of CI checks that are required before merging, and not require the other tests that might have spontaneous failures due to upstream package changes. That'd make it so that weird spontaneous failures are less likely to block PRs from being merged.

Thank you for doing this!

@StanczakDominik
Copy link
Member Author

StanczakDominik commented Jan 10, 2023

  • try out two lockfiles after release

@StanczakDominik StanczakDominik self-assigned this Jan 15, 2023
@StanczakDominik
Copy link
Member Author

I removed temporarily the requirement for the Documentation job, as it's been renamed.

@StanczakDominik
Copy link
Member Author

  • requirements.txt file that combines the two
requirements/docs.txt
requirements/tests.txt
requirements.txt
-r requirements/docs.txt
-r requirements/tests.txt

@StanczakDominik
Copy link
Member Author

Welp. I tried installing the combining requirements.txt file and it already has issues on docutils:

docs.txt:docutils==0.17.1
tests.txt:docutils==0.19

So let's go with a single file.

@StanczakDominik StanczakDominik added CI Related to continuous integration requirements Related to updating requirements labels Jan 18, 2023
Copy link
Member

@namurphy namurphy left a comment

Choose a reason for hiding this comment

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

LGTM! I only have one minor reST edit as a suggestion for this PR. I'm happy to try it out!

- package-ecosystem: pip
directory: /
schedule:
interval: daily # TODO s/daily/weekly once dependabot has settled
Copy link
Member

Choose a reason for hiding this comment

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

It's good to start out with daily for the time being, while making sure that dependabot is working as expected.

I had been leaning towards going with monthly in the long-term, but the more I think about it, weekly would probably be best. In particular, having this be monthly would increase the chances of us having to deal with more than one thing breaking in a single PR. And too often in my experience...doubling the problems that we have to deal with more than doubles the effort needed to fix the combined problems.

changelog/1864.trivial.rst Outdated Show resolved Hide resolved
Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>
@StanczakDominik StanczakDominik enabled auto-merge (squash) January 18, 2023 17:25
@StanczakDominik StanczakDominik merged commit d09cf1a into PlasmaPy:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Related to continuous integration requirements Related to updating requirements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bring back Dependabot
2 participants