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

Minimum version dependency testing #6802

Merged

Conversation

AnOctopus
Copy link
Collaborator

Describe your changes

Updates to the minimum versions of many dependencies, along with a script for generating a constraints file that can be used to install the minimum versions to run tests against.

Testing Plan

These are changes to our dependency version constraints, and testing infrastructure.

@AnOctopus AnOctopus requested a review from tconkling June 5, 2023 22:58
@AnOctopus AnOctopus requested a review from kmcgrady as a code owner June 23, 2023 18:13
Comment on lines +94 to +98
# This section is an inlined copy of the make_init action
# Both because we need to do some parts of it differently
# and because it running in a separate action causes the python path
# setup to work weirdly, so some tests fail unless you run an install
# command in the workflow directly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may have already experimented with this, but just to confirm - adding a new input flag to the make_init action to handle the differences below doesn't work?

List of those I could identify:

  • creating the Python environment cache key (lib/min-constraints-gen.txt vs. lib/dev-requirements.txt)
  • restore virtualenv from cache (changed key name)
  • Create Virtual Env (make python-init-test-min-deps vs. make python-init)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't try doing it as a new input flag, because it felt like it would result in a bad abstraction. While prototyping this, I made changes to make_init directly, and then when I discovered that the path only gets set up correctly if you do an additional install command in the workflow (the existing test workflows do make develop in a step after the make_init action), I decided to inline the action's steps.

Copy link
Contributor

@tconkling tconkling left a comment

Choose a reason for hiding this comment

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

This is great!

As-is, it looks like min-constraints-gen.txt can get stale without us knowing. I don't think this needs to block the PR, but we may want to follow this PR with a CI change that runs make gen-min-dep-constraints in CI and errors if the output is different from the committed file. (We do this with the NOTICES file in the "Validate NOTICES" job in js-tests.yml)

Comment on lines +18 to +29
urllib3>=1.9

hypothesis>=6.17.4
parameterized
pytest
pytest-cov
requests-mock
testfixtures


mypy-protobuf>=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.

Nit: I think the convention here is to alphabetize all dependencies in the list (and also no extra newlines).

Re: the new requirements themselves - it looks like most of these are duplicated from dev-requirements.txt. Should we just be installing dev-requirements.txt for this set of tests, rather than duplicating? Or does that break something with min-dependency testing?

(If the latter, can we add documentation to test-requirements.txt explaining the duplication?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some dev requirements (mainly black) would impose much higher minimum versions of dependencies we share with them, which I wanted to avoid.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Possibly some of these should be factored out into a 3rd requirements file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's also possible that we don't mind requiring a more recent typing extensions version, or could roll back to an older version of black that doesn't need something so recent (don't remember why we're on the version we have).

scripts/get_min_versions.py Outdated Show resolved Hide resolved
scripts/get_min_versions.py Show resolved Hide resolved
Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

+1 for the GHA changes

@AnOctopus AnOctopus merged commit c7f9791 into streamlit:develop Jun 27, 2023
49 checks passed
@AnOctopus AnOctopus deleted the feature/minimum-dependency-version-testing branch June 27, 2023 00:03
tconkling added a commit to tconkling/streamlit that referenced this pull request Jun 27, 2023
* develop:
  Minimum version dependency testing (streamlit#6802)
  Delay slider `thumbValueAlignment` to allow page layout to complete (streamlit#6828)
eric-skydio pushed a commit to eric-skydio/streamlit that referenced this pull request Dec 20, 2023
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Mar 22, 2024
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants