Skip to content

Use constraint files to install dependencies on PRs #6396

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.

Sign up for GitHub

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

Jump to bottom
Merged
sfc-gh-mnowotka merged 4 commits into from
Apr 12, 2023
Merged

Use constraint files to install dependencies on PRs #6396

sfc-gh-mnowotka merged 4 commits into from
Apr 12, 2023
+57 −25

Conversation

@sfc-gh-kbregula
Copy link
Contributor

@sfc-gh-kbregula sfc-gh-kbregula commented Mar 30, 2023 •
edited
Loading

📚 Context

This change introduces two changes:

  • introduces the concept of the canary build.
  • uses constraints files when installing dependencies for non-canary builds

Canary build

Canary build is a new concept, but these are PR builds that aim to quickly spot dependency issues or incompatibilities between different versions of Python.

A build is canary if one of the following conditions is met.

  • The Github event is a pull request and dependencies have been modified
  • The Github event is a push, the default branch(develop) is checked.

All other build is non-canary build.

Constraints files
For pull requests or for local installation, constraint files will now be used by default. This will improve stability as we will be using dependencies that have already been proven to work. Canary build will always install the latest dependencies.

To force install the latest dependencies:

  • For PR, we can use the PR dev:upgrade-dependencies label.
  • For a local installation, we can use the environment variable USE_CONSTRAINT_FILE=false.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-use-in-prs branch from f7c88d0 to 2ff1b2a Compare March 30, 2023 10:18
@sfc-gh-kbregula sfc-gh-kbregula mentioned this pull request Apr 4, 2023
Merged
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-use-in-prs branch from 2ff1b2a to 0ca812a Compare April 5, 2023 11:12
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-use-in-prs branch from fdecf44 to 24f6435 Compare April 6, 2023 07:52
@sfc-gh-kbregula sfc-gh-kbregula marked this pull request as ready for review April 6, 2023 07:56
@sfc-gh-kbregula sfc-gh-kbregula force-pushed the sfc-gh-kbregula-constraints-use-in-prs branch from bd4ca34 to 47f5fe4 Compare April 6, 2023 16:39
vdonato
vdonato approved these changes Apr 7, 2023
View reviewed changes
Copy link
Collaborator

@vdonato vdonato left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This LGTM + I'd be in favor of making this change!

I think we'll also want to get some other opinions from the team, though, because I could see an argument for never using constraint files when running CI to ensure that we maximize the probability that a breaking dependency is caught as quickly as possible, even if it does result in more pain for us (one could imagine a situation where we miss a breaking dependency for a full day or so because it's a slow day and no merges into develop occur).

In this case, I think that this change strikes the right balance between us being able to keep PRs moving in case a dependency does break and still being able to catch issues as they arise, but it'd be worth seeing what others think.

@sfc-gh-kbregula
Copy link
Contributor Author

sfc-gh-kbregula commented Apr 11, 2023

@vdonato scheduled builds never use constraints files, so we have at least 1 build every day that uses the latest dependencies.

With this change, I would like to solve the problem that broken dependencies that have not been modified in any way hinder the work on independent changes.
Example build: https://github.com/streamlit/streamlit/actions/runs/4600656271/jobs/8127530846?pr=6174
This PR is open for quite a while, but does not modify the dependency in any way. I sometimes do a rebase to keep it merge-ready, but unrelated dependency issues make it difficult because I have to rebase multiple times without code changes.

I will ask other people what they think about this change.

@sfc-gh-kbregula sfc-gh-kbregula added the security-assessment-completed Security assessment has been completed for PR label Apr 12, 2023
@sfc-gh-mnowotka sfc-gh-mnowotka merged commit c2d16a9 into develop Apr 12, 2023
@sfc-gh-kmcgrady sfc-gh-kmcgrady deleted the sfc-gh-kbregula-constraints-use-in-prs branch October 5, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdonato vdonato vdonato approved these changes

@mayagbarnes mayagbarnes Awaiting requested review from mayagbarnes

@kmcgrady kmcgrady Awaiting requested review from kmcgrady

@sfc-gh-mnowotka sfc-gh-mnowotka Awaiting requested review from sfc-gh-mnowotka

Assignees

No one assigned

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sfc-gh-kbregula @vdonato @sfc-gh-mnowotka