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

pre-commit: autorun uv lock if --check fails #6711

Closed
wants to merge 2 commits into from

Conversation

khsrali
Copy link
Contributor

@khsrali khsrali commented Jan 16, 2025

why not?

@khsrali khsrali changed the title pre-commit: auto run uv lock if --check fails pre-commit: autorun uv lock if --check fails Jan 16, 2025
@unkcpz
Copy link
Member

unkcpz commented Jan 16, 2025

Is this to fix the failed CI, why they are failed now? Is this one covered by #6699? @danielhollas

@khsrali
Copy link
Contributor Author

khsrali commented Jan 16, 2025

Is this to fix the failed CI, why they are failed now? Is this one covered by #6699? @danielhollas

CI fails for different reasons, probably because #6712
I wasn't aware of #6699, will have a look on that 👀

@danielhollas
Copy link
Collaborator

Yes, #6699 should cover this.

For the failing CI, please update your uv version and then try to update the lock.
There were changes in the version from yesterday for handling of dynamic version (which we do have in our pyproject.toml)

@unkcpz
Copy link
Member

unkcpz commented Jan 16, 2025

The latest uv version now is 0.5.20, I update and rerun uv lock --upgrade. The CI tests are fine now but the readthedocs build failed on uv sync --extra docs --extra tests --extra rest --extra atomic_tools with:

error: Failed to parse `uv.lock`
  Caused by: TOML parse error at line 25, column 1
   |
25 | [[package]]
   | ^^^^^^^^^^^
missing field `version`

I assume the readthedocs build config should also sync the uv version.

@danielhollas
Copy link
Collaborator

danielhollas commented Jan 16, 2025 via email

@unkcpz
Copy link
Member

unkcpz commented Jan 16, 2025

Thanks for quick reply @danielhollas
Would you mind I bump uv version in RTD in my other PR and merge? Or you prefer to wait until it covered by an independent solution?

@danielhollas
Copy link
Collaborator

Would you mind I bump uv version in RTD in my other PR and merge? Or you prefer to wait until it covered by an independent solution?

Go for it.

Copy link
Collaborator

@danielhollas danielhollas left a comment

Choose a reason for hiding this comment

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

I'd prefer if we could fix this in #6699 since that one can also enforce a specific uv version. Maybe let's leave this as a draft as a backup plan if #6699 cannot be merged in a reasonable timeframe.

@@ -189,8 +189,8 @@ repos:

- id: check-uv-lock
name: Check uv lockfile up to date
# NOTE: This will not automatically update the lockfile
entry: uv lock --check
entry: bash -c 'uv lock --check || uv lock'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure it this works correctly, because we need to fail the pre-commit check if the lockfile is updated, but based on my limited testing uv lock returns 0 regardless if the lockfile was updated or not.

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 guess with --check return zero only if lockfile is already updated.
I ran a few manual tests, for me it works.

@khsrali
Copy link
Contributor Author

khsrali commented Jan 16, 2025

I'd prefer if we could fix this in #6699 since that one can also enforce a specific uv version. Maybe let's leave this as a draft as a backup plan if #6699 cannot be merged in a reasonable timeframe.

yes, absolutely! I wasn't aware of your PR before opening this :-)

@khsrali
Copy link
Contributor Author

khsrali commented Jan 16, 2025

@danielhollas now that the minimum version is set,
maybe we can take this temporary solution to have a cautious pre-commit
I'm not aware how long it takes for uv to merge their thing

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.00%. Comparing base (c88fc05) to head (cc5221e).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6711   +/-   ##
=======================================
  Coverage   78.00%   78.00%           
=======================================
  Files         563      563           
  Lines       41766    41766           
=======================================
  Hits        32574    32574           
  Misses       9192     9192           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielhollas
Copy link
Collaborator

Well, the bug that I reported yesterday that was blocking us is already fixed. 😄
astral-sh/uv#10689

So let's wait for next release, which will most likely happen sometimes next week, they release very often.

@khsrali
Copy link
Contributor Author

khsrali commented Jan 22, 2025

solution #6699 is preferred.

@khsrali khsrali closed this Jan 22, 2025
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.

3 participants