fix(python): install workspace prerequisites before instrumentors - #3744
Open
mikeldking wants to merge 2 commits into
Open
mikeldking wants to merge 2 commits into
mikeldking wants to merge 2 commits into
Conversation
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Comment on lines
+299
to
+300
| uv pip install --reinstall {toxinidir}/openinference-semantic-conventions | ||
| uv pip install --reinstall {toxinidir}/openinference-instrumentation |
Contributor
There was a problem hiding this comment.
The trailing unconditional reinstall clobbers the [test] extras for the instrumentation environment.
In the new ordering, the execution sequence for the instrumentation tox environment is:
- Line ~107:
uv pip install --reinstall {toxinidir}/openinference-instrumentation(unconditional — no extras) - Line ~256:
instrumentation: uv pip install --reinstall {toxinidir}/openinference-instrumentation[test](factor-gated — installs[test]extras ✓) - Line 300:
uv pip install --reinstall {toxinidir}/openinference-instrumentation(unconditional — overwrites without[test]extras ✗)
In the original code, the two unconditional installs were in the middle of commands_pre, so the factor-gated [test] install at line 256 came last and won. Now they're bookends, and the trailing unconditional (line 300) runs after the [test] install, stripping the extras.
Fix: add a factor-gated reinstall after the unconditional trailing line so the instrumentation environment ends up with [test] extras:
Suggested change
| uv pip install --reinstall {toxinidir}/openinference-semantic-conventions | |
| uv pip install --reinstall {toxinidir}/openinference-instrumentation | |
| uv pip install --reinstall {toxinidir}/openinference-semantic-conventions | |
| uv pip install --reinstall {toxinidir}/openinference-instrumentation | |
| instrumentation: uv pip install --reinstall {toxinidir}/openinference-instrumentation[test] |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3743.
Install checked-out semantic conventions and core instrumentation as bookends around leaf-specific setup in tox
commands_pre:UV_EXCLUDE_NEWERhides recent PyPI releases;Verification:
py310-ci-agno-latest: 147 tests passedpy314-ci-mistralai-latest: 25 tests passedopeninference-instrumentation==0.1.63.