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

use the lock file for pinning dependencies, not the project definition #18305

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

altendky
Copy link
Contributor

@altendky altendky commented Jul 15, 2024

Purpose:

Presently every dependabot PR that updates a package we have specified in pyproject.toml conflicts because the poetry.lock has a hash of the contents of pyproject.toml. There's various discussion that could be had around * vs. >=x.y.z vs. ^x.y and specifying versions here or there or wherever... but we can't be dealing with these conflicts every time so some option involving not changing pyproject.toml every time is needed.

With this change it is reasonable once again to have dependabot auto-rebase since the PRs won't all be conflicting.

Current Behavior:

New Behavior:

Testing Notes:

@altendky altendky added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jul 15, 2024
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 15, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 26, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@altendky altendky marked this pull request as ready for review July 26, 2024 17:30
@altendky altendky requested a review from a team as a code owner July 26, 2024 17:30
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 29, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Aug 12, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@altendky altendky marked this pull request as draft August 14, 2024 14:12
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Aug 15, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot added merge_conflict Branch has conflicts that prevent merge to main and removed merge_conflict Branch has conflicts that prevent merge to main labels Sep 11, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

keyring = "25.2.1" # Store keys in MacOS Keychain, Windows Credential Locker
packaging = "24.0"
pip = "24.2"
aiofiles = ">=24.1.0" # Async IO for files
Copy link
Contributor

@arvidn arvidn Sep 12, 2024

Choose a reason for hiding this comment

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

as I understand the poetry documentation, we would want to use ^ instead of >=.
https://python-poetry.org/docs/dependency-specification/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I personally started with * since we have no particular ties the the version we are exactly pinning now. I switched to >= since Earle asked for it. There's doubt in the Python community that upper version limits actually net a better ecosystem. Yeah, SemVer, I'm all about it. But, if you strictly follow it then you can end up with many major version bumps over minor corner case fixes that won't affect the vast majority of dependees. Then some of those dependees aren't really well maintained and have upper version limits. Then nobody can install anything sensible anymore. The flip side is more accidental breakage... but. Anyone can add a constraint (an upper limit for a package) while nobody can take an existing constraint away from a package they don't own (like an upper limit). And yes, there are issues talking about adding the ability to override other package's dependency constraints. :]

So yes, we could ^ if we wanted to constrain the upper bound. Given that we have a lock and require PRs, we'll be testing every update to the lock the same regardless of it being major, minor, or patch and also regardless of what constraint we use here.

There are cases, like a user needing to do a local version update for whatever reason, where better describing our limits of compatibility would be good. Today, we provide no guidance on that since we pin version in both the lock and the project definition. So, I'm not clear that just dropping the constraints in the project definition makes things particularly worse.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Sep 12, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot added merge_conflict Branch has conflicts that prevent merge to main labels Sep 13, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Sep 25, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Sep 26, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Sep 27, 2024
Copy link

Pull Request Test Coverage Report for Build 11072836265

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 37 unchanged lines in 7 files lost coverage.
  • Overall coverage remained the same at 90.958%

Files with Coverage Reduction New Missed Lines %
chia/data_layer/data_store.py 1 95.64%
chia/_tests/core/full_node/test_transactions.py 1 99.14%
chia/wallet/util/wallet_sync_utils.py 1 86.54%
chia/plotters/madmax.py 1 50.6%
chia/rpc/rpc_server.py 4 87.83%
chia/wallet/wallet_node.py 7 88.15%
chia/_tests/core/util/test_lockfile.py 22 77.73%
Totals Coverage Status
Change from base Build 11060322104: 0.0%
Covered Lines: 102031
Relevant Lines: 112151

💛 - Coveralls

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Sep 30, 2024
Copy link
Contributor

This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed by the relevant parties.

@github-actions github-actions bot added the stale-pr Flagged as stale and in need of manual review label Nov 15, 2024
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Nov 26, 2024
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@altendky altendky marked this pull request as ready for review November 26, 2024 21:02
@altendky altendky requested a review from a team as a code owner November 26, 2024 21:02
@altendky altendky removed the stale-pr Flagged as stale and in need of manual review label Nov 26, 2024
@altendky altendky marked this pull request as draft November 26, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants