Skip to content

build: Fix pin of opentelemetry-api and update lock file#16

Closed
ko3n1g wants to merge 6 commits intoNVIDIA:mainfrom
ko3n1g:main
Closed

build: Fix pin of opentelemetry-api and update lock file#16
ko3n1g wants to merge 6 commits intoNVIDIA:mainfrom
ko3n1g:main

Conversation

@ko3n1g
Copy link

@ko3n1g ko3n1g commented Jul 19, 2025

Summary

Add a one line overview of what this PR aims to accomplish.

Details

Describe your changes. You can be more detailed and descriptive here.

Usage

You can potentially add a usage example below.

# Add a code snippet demonstrating how to use this.

Before your PR is "Ready for review"

  • Make sure you read and followed Contributor guidelines
  • Did you write any new necessary tests?
  • Did you add or update any necessary documentation?
  • Did you add notable changes to the client (i.e. not related to tooling, CI/CD, etc.) from this PR to .release_notes/.unreleased.md?

Additional Information

  • Related to issue link (an issue is needed to notify us on Slack).

ko3n1g added 6 commits July 19, 2025 17:05
Signed-off-by: oliver könig <okoenig@nvidia.com>
Signed-off-by: oliver könig <okoenig@nvidia.com>
Signed-off-by: oliver könig <okoenig@nvidia.com>
Signed-off-by: oliver könig <okoenig@nvidia.com>
Signed-off-by: oliver könig <okoenig@nvidia.com>
Signed-off-by: oliver könig <okoenig@nvidia.com>
@commiterate
Copy link
Collaborator

commiterate commented Jul 21, 2025

This will restrict opentelemetry-api to 1.24.* since ~=1.24.0 is equivalent to >=1.24.0,==1.24.* while ~=1.24 is equivalent to >=1.24,==1.* (PyPA docs). PyPA's ~= is NOT equivalent to Poetry's ^.

The current ~=1.24 fits the intention of allowing $version \in [1.24, 2)$. If anything, we need to swap some of the compatible release specifiers with the patch version to a different form.

Exact versions for our own testing are already locked by uv.lock.

@ko3n1g
Copy link
Author

ko3n1g commented Jul 21, 2025

This will lock opentelemetry-api to 1.24.* since ~=1.24.0 is equivalent to >=1.24.0,==1.24.* while ~=1.24 is equivalent to >=1.24,==1.* (PyPA docs).

The current ~=1.24 fits the intention of allowing v e r s i o n ∈ [ 1.24 , 2 ) . If anything, we need to swap some of the compatible release specifiers with the patch version to a different form.

Exact versions for our own testing are already locked by uv.lock.

Hey yeah, sorry, didn't have time to continue this. Originally I thought I had seen a breakage on opentelemetry 1.35, but that one was a fluke.

But I think this PR is still relevant to some extend, since upgrading the lock-file shouldn't break the CI. I think this might give evidence that some recent upstream dependencies aren't compatible with msc anymore.

@commiterate
Copy link
Collaborator

commiterate commented Jul 21, 2025

https://github.com/NVIDIA/multi-storage-client/actions/runs/16391406807/job/46317884014?pr=16#step:4:619

I'm not too sure why that's failing. It doesn't seem like an issue with an upgraded dependency.

We can test the PR by mirroring internally to a GitLab MR to test using our CI there (long-term plan is to move development to GitHub). Though I'd be surprised if the result changed since we're using Nix shells everywhere.

@commiterate
Copy link
Collaborator

commiterate commented Jul 21, 2025

Anyways the exact changes for this PR probably aren't needed, but we need to change any ~={major}.{minor}.{patch} statements since they're unintentionally restricting packages to a specific {major}.{minor} version.

We'll create an MR for this on GitLab directly.

@ko3n1g
Copy link
Author

ko3n1g commented Jul 21, 2025

Anyways the exact changes for this PR probably aren't needed, but we need to change any ~={major}.{minor}.{patch} statements since they're unintentionally restricting packages to a specific {major}.{minor} version.

We'll create an MR for this on GitLab directly.

Yeah, I wasn't sure about your preference:) If pinning majors works for you, that's great. Pinning minors seems safer but also improses more limitations on users. In any case, I'd strongly recommend to have a weekly uv-lock-upgrade job in place that verifies that you're still compatible against the upper boundaries (however they are defined)

@commiterate
Copy link
Collaborator

We've actually made uv intentionally resolve the lowest allowed versions of direct dependencies to make sure the lower end of our version ranges work.

resolution = "lowest-direct"

Ideally this works fine, but it's possible for upstream dependencies to break semver.

@commiterate
Copy link
Collaborator

Followup GitLab MR opened. Closing this PR.

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.

2 participants