Skip to content

Conversation

pdgendt
Copy link
Contributor

@pdgendt pdgendt commented Mar 21, 2025

It is not recommended by the Python Packaging User Guide to pin dependencies. This is a naive approach to set the project's pinned dependencies as the package's minimal dependencies.

Fixes #4500

@pdgendt pdgendt requested review from bruntib and vodorok as code owners March 21, 2025 09:42
Copy link
Contributor

@bruntib bruntib left a comment

Choose a reason for hiding this comment

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

Hi,
Thanks for the change. However, I wouldn't agree with changing dependency versions compared to what we describe in requirements.txt files. There should be a single source of dependencies and versions.
Also, I would say that at only minor version changes should be allowed in an automatic manner.

@pdgendt
Copy link
Contributor Author

pdgendt commented Mar 21, 2025

Hi, Thanks for the change. However, I wouldn't agree with changing dependency versions compared to what we describe in requirements.txt files. There should be a single source of dependencies and versions. Also, I would say that at only minor version changes should be allowed in an automatic manner.

Yes, as stated, this was a very naive attempt, but I still think the referred issue is a valid one. Any alternative suggestions?

@bruntib
Copy link
Contributor

bruntib commented Mar 21, 2025

Actually, requiring specific versions has historical reasons. In the beginning of the project's lifecycle it caused a lot of headache to detect whether an issue is due to a dependency version mismatch, or CodeChecker itself. So, we decided to use specific version numbers so it is common for everyone.
I wouldn't be against trying to be more flexible in this question, but I would suggest the following things:

  • Dependency version numbers should be maintained in requirements.txt files only. I wouldn't think that using CodeChecker in different contexts should come with different version numbers (i.e. having CodeChecker via "pip install" vs. "make package").
  • The minimum version numbers should be provided in this format: lxml~=5.3. This is the same as: lxml>=5.3,<6. Major version change may result backward incompatible change and it wouldn't be nice if such a change would break the CI. https://packaging.python.org/en/latest/specifications/version-specifiers/#compatible-release

@pdgendt pdgendt force-pushed the setup-req-minimal branch 2 times, most recently from 2d19175 to 7aadfc2 Compare March 21, 2025 14:45
@pdgendt
Copy link
Contributor Author

pdgendt commented Mar 21, 2025

I wouldn't be against trying to be more flexible in this question

Great!

  • Dependency version numbers should be maintained in requirements.txt files only. I wouldn't think that using CodeChecker in different contexts should come with different version numbers (i.e. having CodeChecker via "pip install" vs. "make package").

It is common however to use tooling (pip-tools for example) to "fix" testing with a known and reproducible version, but not imposing it onto the end-user.

I've the updated the PR, is this how you prefer the changes?

@bruntib
Copy link
Contributor

bruntib commented Mar 21, 2025

There are many other files, too. Otherwise the change looks fine. Thank you!

./tools/tu_collector/requirements_py/dev/requirements.txt
./tools/bazel/requirements_py/dev/requirements.txt
./tools/report-converter/requirements_py/dev/requirements.txt
./web/requirements_py/db_psycopg2/requirements.txt
./web/requirements_py/osx/requirements.txt
./web/requirements_py/auth/requirements.txt
./web/requirements_py/dev/requirements.txt
./web/requirements_py/db_pg8000/requirements.txt
./web/requirements.txt
./requirements_py/docs/requirements.txt
./analyzer/tools/build-logger/requirements_py/dev/requirements.txt
./analyzer/tools/merge_clang_extdef_mappings/requirements_py/dev/requirements.txt
./analyzer/tools/statistics_collector/requirements_py/dev/requirements.txt
./analyzer/requirements_py/osx/requirements.txt
./analyzer/requirements_py/dev/requirements.txt
./analyzer/requirements.txt
./codechecker_common/requirements_py/dev/requirements.txt

@pdgendt pdgendt force-pushed the setup-req-minimal branch from 7aadfc2 to ddfb19b Compare March 21, 2025 15:35
@pdgendt pdgendt changed the title [cfg] Use minimal versions instead of pinned for setuptools [refactor] Use minimal versions instead of pinned for setuptools Mar 21, 2025
@pdgendt
Copy link
Contributor Author

pdgendt commented Mar 21, 2025

There are many other files, too. Otherwise the change looks fine. Thank you!

Sure, didn't know it was good if I touched all those files, instead of the ones used for pypi packaging. Updated

@bruntib
Copy link
Contributor

bruntib commented Mar 24, 2025

Now it occurred that an automatic upgrade of a dependency (mypy in this case) caused a failing CI. I'm starting to be skeptic with the usefulness of the recommendation of auto version upgrade. It feels like it will cause more headache in the future.
Shouldn't we rather stay with the current way of using specific 3pp versions and upgrade them right before each release? We could add this action to a check-list before the releases.

@elupus
Copy link
Contributor

elupus commented Mar 24, 2025

Normally you pin ci versions, but not the package versions.

@elupus
Copy link
Contributor

elupus commented Mar 24, 2025

Ps. Its very very useful in others ci environment which may pull in stuff like sphinx to build docs and other tools.

If codechecker pins its packages, these will conflict with other projects pins. Which is why it should not be pinned in the package description.

@elupus
Copy link
Contributor

elupus commented Mar 24, 2025

Also. I you dont use Poetry/uv lock files or pip compile to ensure a stable CI environment. You are still susceptible to sub package dependency upgrades.

@pdgendt
Copy link
Contributor Author

pdgendt commented Mar 25, 2025

Normally you pin ci versions, but not the package versions.

Exactly, moreover you pin using hashes to avoid tampered versions.

Now it occurred that an automatic upgrade of a dependency (mypy in this case) caused a failing CI. I'm starting to be skeptic with the usefulness of the recommendation of auto version upgrade. It feels like it will cause more headache in the future. Shouldn't we rather stay with the current way of using specific 3pp versions and upgrade them right before each release? We could add this action to a check-list before the releases.

@bruntib what's the plan then? Is there a clear distinction between package dependencies vs testing dependencies?

One approach I can recommend is using tooling to pin the dependencies for you. You provide a requirements.in file that has the minimal and/or excluded versions, and run

$  pip-compile --generate-hashes --output-file=requirements.txt requirements.in

The requirements.txt file is the source of truth for CI, and requirements.in for package depedencies.

Next to that, add a dependabot configuration that checks for updates, which can be tested in CI before pushing to the main branch.

@pdgendt
Copy link
Contributor Author

pdgendt commented May 2, 2025

@bruntib any more feedback on this?

I often need to uninstall CodeChecker because it forces versions of dependencies for an entire environment, which isn't ideal. Projects should pin in CI and during testing, so users can always fallback to those if issues arise.

@pdgendt pdgendt force-pushed the setup-req-minimal branch from df7fc9d to 2309628 Compare May 5, 2025 09:04
@bruntib
Copy link
Contributor

bruntib commented Jun 10, 2025

@pdgendt, Sorry for disappearing this long. Alright, let's try using the ~= in requirements.txt files, like in the current version of this PR. Could you, please, resolve the conflicts, and then we could try this approach. Please, also make sure that the required tests are passing. Thank you!

@pdgendt pdgendt force-pushed the setup-req-minimal branch from 2309628 to a6a1339 Compare June 11, 2025 07:24
It is not recommended by the Python Packaging User Guide to pin
dependencies.

Signed-off-by: Pieter De Gendt <[email protected]>
@pdgendt pdgendt force-pushed the setup-req-minimal branch from a6a1339 to eda2fa4 Compare June 11, 2025 13:52
@elupus
Copy link
Contributor

elupus commented Jul 1, 2025

@pdgendt seems some checks are failing?

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.

Python Package dependencies should not be pinned
3 participants