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

Add Dependabot to the repository #275

Merged
merged 16 commits into from
Jul 4, 2022
Merged

Add Dependabot to the repository #275

merged 16 commits into from
Jul 4, 2022

Conversation

PProfizi
Copy link
Contributor

No description provided.

@PProfizi PProfizi added the maintenance Repository structure maintenance label Jun 30, 2022
@PProfizi PProfizi self-assigned this Jun 30, 2022
Add management of action versions for github-actions. Update directory for pip ecosystem.
@PProfizi PProfizi requested a review from akaszynski June 30, 2022 16:06
@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #275 (3ba9dff) into master (faf54c0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #275   +/-   ##
=======================================
  Coverage   86.20%   86.20%           
=======================================
  Files          52       52           
  Lines        5661     5661           
=======================================
  Hits         4880     4880           
  Misses        781      781           

@akaszynski
Copy link
Contributor

You'll need to "pin" the versions of the requirements in all your requirements files for dependabot to work.

@PProfizi
Copy link
Contributor Author

PProfizi commented Jul 1, 2022

@akaszynski Oh so Dependabot will only deal with pinned down dependencies. The thing is we do not necessarily want to pin down all of our dependencies. I understand this is maybe best practice, but IMO it might create more versioning conflicts when someone tries to install our package along with another one.
I discussed this with other DPF members and the feeling is that we might want to actually manage a "=<" version for each package. This way a released package gives a range of compatible dependencies.
Does that make sense?
TLDR; not sure how pinning down every dependency solves more problems than it creates.

@akaszynski
Copy link
Contributor

@akaszynski Oh so Dependabot will only deal with pinned down dependencies. The thing is we do not necessarily want to pin down all of our dependencies. I understand this is maybe best practice, but IMO it might create more versioning conflicts when someone tries to install our package along with another one. I discussed this with other DPF members and the feeling is that we might want to actually manage a "=<" version for each package. This way a released package gives a range of compatible dependencies. Does that make sense? TLDR; not sure how pinning down every dependency solves more problems than it creates.

The thing is we do not necessarily want to pin down all of our dependencies.

Agreed 100%. PyAnsys packages should have loose requirements (or as loose as possible) to ensure multiple packages can work in the same environment.

IMO it might create more versioning conflicts when someone tries to install our package along with another one.

Absolutely. Anything in your setup.py or pyproject.toml that's actually required by your package should allow the oldest version possible to run your library.

I discussed this with other DPF members and the feeling is that we might want to actually manage a "=<" version for each package.

That's one way of ensuring that upstream dependencies don't break your current project. However, even though it's a potential risk I'd instead only a minimum version requirement. For example, vtk>=9.0.

So, at this point you're probably asking, why use dependabot at all since, as you stated, it only works with "pinned down" dependencies.

Oh so Dependabot will only deal with pinned down dependencies.

It works with fully pinned or those with "upper limits" like pyvista <= 0.34.0, which we've established isn't the best practice for run-time dependencies. However, and perhaps it's because I wasn't clear, the only requirements you should pin down should be your testing and documentation dependencies.

By pinning down requirements_test.txt to the latest dependencies, you can ensure that your package works with the latest version of a package as soon as it's released to PyPI. This ensures "the best of both worlds", you can allow for the "latest" package in your install_requires section of setup.py, but you always test with the latest package in your CI/CD. The only thing dependabot does is create PRs that bump your requirements_*.txt dependencies and the workflow will either run or not.

This gives you a chance to either patch the issue due to the upstream changes, or provide an upper bound for your install_requires for that package until you patch it. Either way, you'll have to patch it, and it's much better for a "bot" to tell you that your library can't support the latest version of <some-dependency> rather than your users, because users will always be trying to use the latest version of everything.

Ping me on teams if we need to discuss this further, or if our docs at https://dev.docs.pyansys.com/ need to include this discussion. (actually, there already is an issue at ansys/pyansys-dev-guide#125).

@PProfizi PProfizi closed this Jul 1, 2022
@PProfizi PProfizi reopened this Jul 1, 2022
@PProfizi PProfizi added the dependencies Related to package requirements label Jul 1, 2022
@akaszynski
Copy link
Contributor

FYI: Dependabot won't trigger until this PR is merged.

@@ -1,7 +1,7 @@
pytest==7.1.2
pytest-cov==3.0.0
pytest-rerunfailures==10.2
matplotlib==3.5.2
matplotlib==3.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Dependabot will try to upgrade this BTW. If something's breaking, we should support it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akaszynski Yes, I also think it is a bad idea to restrain the version of matplotlib. This was to check whether the pipeline failure came from this (apparently it does not).

@PProfizi PProfizi merged commit 4a10daa into master Jul 4, 2022
@PProfizi PProfizi deleted the maint/add_dependabot branch July 4, 2022 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Related to package requirements maintenance Repository structure maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants