[NA] [SDK] fix(harbor): support both _setup_environment and _setup_agent_environment#6834
Merged
Merged
Conversation
…ironment harbor >= 0.8.0 renamed Trial._setup_environment to Trial._setup_agent_environment. The integration's _enable_harbor_tracking patched the old name, so any user calling track_harbor() against current harbor hit AttributeError on the hasattr/setattr lookup at opik_tracker.py:278. Library-integration tests (test_track_harbor_patches_trial_setup_environment et al.) caught it but the production code path was equally broken — users couldn't enable tracing. Pure attribute rename: the wrapper signature (self: Trial) -> None is unchanged. Span name in the tracker also bumped from "setup_environment" to "setup_agent_environment" so traces accurately reflect the upstream method being called. Test file's expected_methods list and the per-method patch assertion updated to match. The TestHarborClassesExist test (which exists specifically to flag upstream renames) now reflects the post-rename API surface.
…environment (>=0.8) Previous commit on this branch swapped the patched attribute name from _setup_environment to _setup_agent_environment, which fixed harbor >= 0.8 but broke users still on harbor < 0.8. Patch whichever name the installed version exposes instead. opik_tracker.py: - New _patch_method_if_present helper does a getattr/hasattr/setattr cycle guarded by the opik_tracked sentinel. It silently skips when the attribute is missing, so we can list both candidate names without worrying about which version is installed. - _enable_harbor_tracking iterates _TRIAL_SETUP_ENVIRONMENT_METHOD_NAMES and calls the helper for each. - The wrapper function reverts to _wrap_setup_environment (one function handles both attribute names — the underlying (self) -> None signature is identical across versions). Span name reverts to "setup_environment" so existing dashboards keep working regardless of harbor version. test_harbor.py: - Each per-method patching test is guarded by pytest.skipif on the attribute's presence: the < 0.8 test skips on harbor >= 0.8 and vice versa, so the suite is green on either version. - test_trial_class_has_expected_methods asserts presence of one of the two environment-setup method names (whichever is shipped) rather than hard-coding either.
github-actions
Bot
added
python
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
+60
to
+64
| def test_track_harbor_patches_trial_setup_agent_environment(self): | ||
| """Verify Trial._setup_agent_environment method is patched (harbor >= 0.8.0).""" | ||
| track_harbor() | ||
| assert hasattr(Trial._setup_agent_environment, "opik_tracked"), ( | ||
| "Trial._setup_agent_environment should have opik_tracked attribute after track_harbor()" |
Contributor
There was a problem hiding this comment.
This test asserts hasattr(Trial._setup_agent_environment, "opik_tracked") — a private impl detail — and doesn't match the naming convention in .agents/skills/python-sdk/testing.md; should we rename it and switch to a fake_backend.trace_trees assertion like test_track_harbor_sets_correct_trace_name?
Finding types: AI Coding Guidelines | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
In sdks/python/tests/library_integration/harbor/test_harbor.py around lines 60-64: 1.
Rename `test_track_harbor_patches_trial_setup_agent_environment` to follow the
`test_<WHAT>__<CASE_DESCRIPTION>__<EXPECTED_RESULT>` or `__happyflow` pattern from
`.agents/skills/python-sdk/testing.md`, clearly expressing that `track_harbor()` is
being tested for correct tracing behavior when harbor >= 0.8.0. 2. Replace the
`hasattr(Trial._setup_agent_environment, "opik_tracked")` assertion with an externally
observable check (e.g., `fake_backend.trace_trees`) similar to
`test_track_harbor_sets_correct_trace_name`, so the test validates public behavior
rather than private implementation details and remains resilient to internal refactors.
Keep the existing `@pytest.mark.skipif` condition and ensure no other references to the
old function name remain.
…present Previous attempt only routed _setup_environment / _setup_agent_environment through the existence-checking helper; the other four Trial method patches still did direct `Trial._execute_agent` etc. attribute access. harbor 0.8.0 dropped _execute_agent entirely, so track_harbor() on harbor >= 0.8.0 still crashed with AttributeError at the first non-routed line — and because all the per-method patch tests call track_harbor() in setUp, every single TestTrackHarborPatching test failed, not just the env-setup one. Move the full Trial-method patch list into a single tuple iterated through _patch_method_if_present, so any method harbor renames or drops is silently skipped. The constant lives inside _enable_harbor_tracking now so it can reference the wrappers (which are defined below in the file) directly, without globals() indirection. test_harbor.py: - Each per-method patching test (setup_agent, execute_agent, run_verification) now has the same pytest.skipif guard as the env-setup tests. - TestHarborClassesExist split into two assertions: Trial.run is the only truly-load-bearing hook; for the rest we assert at least one of the historically-patched method names is still present so we know if the integration silently becomes a no-op against a future harbor release.
Comment on lines
+150
to
154
| known_method_names = ( | ||
| "_setup_environment", | ||
| "_setup_agent_environment", | ||
| "_setup_agent", | ||
| "_execute_agent", |
Contributor
There was a problem hiding this comment.
known_method_names only covers the internal hooks, but _enable_harbor_tracking() also patches Trial.run via trial_patch_targets. Should we include run in the no-op check, or drop this assertion?
Finding type: Logical Bugs | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
sdks/python/tests/library_integration/harbor/test_harbor.py around lines 146-162, inside
`test_trial_class_has_at_least_one_known_method`, the `known_method_names` tuple only
checks internal `_setup*`/`_execute_agent`/`_run_verification` hooks, but
`_enable_harbor_tracking()` always patches `Trial.run`. Update the test so the
“integration is a no-op” assertion accounts for `Trial.run` being present (either
include `"run"` in `known_method_names` or change the assertion to specifically fail
only when neither `Trial.run` nor any internal hook exists). Then update the assertion
message accordingly so it doesn’t claim the integration is a no-op when tracing still
occurs via `Trial.run`.
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
1 participant
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.
Details
harbor 0.8.0 renamed
Trial._setup_environmenttoTrial._setup_agent_environment. The integration's_enable_harbor_trackingpatched the old name, so users on harbor ≥ 0.8 hitAttributeErroron thehasattr/setattrlookup inopik_tracker.pyandtrack_harbor()was effectively broken for them — not just in CI tests but for real users in production. A naive rename to the new name would break users still on harbor < 0.8. This PR patches whichever name the installed version exposes._patch_method_if_presenthelper does thegetattr/hasattr opik_tracked/setattrcycle guarded by a sentinel and silently skips when the attribute is missing._enable_harbor_trackingiterates_TRIAL_SETUP_ENVIRONMENT_METHOD_NAMES = ("_setup_environment", "_setup_agent_environment")calling the helper for each — exactly one matches per installed version._wrap_setup_environment) handles both: underlying(self) -> Nonesignature is identical across versions. Span name stayssetup_environmentso existing dashboards keep working regardless of harbor version.pytest.skipifon the attribute's presence; the env-setup test pair covers both legacy and new harbor.test_trial_class_has_expected_methodsaccepts either method name.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
opik_tracker.py; skipif markers intest_harbor.pypytest tests/library_integration/harbor/test_harbor.pyagainst locally-installed harbor 0.4.0 (13 passed, 1 skipped). CI on harbor 0.8.0 will see the mirror: legacy test skipped, new test runs.Testing
Local against harbor 0.4.0:
The skipped one is
test_track_harbor_patches_trial_setup_agent_environment, which is gated onhasattr(Trial, "_setup_agent_environment").This branch is the surgical follow-up to a CI failure spotted on PR #6829. The actual user-facing fix is in
opik_tracker.py; the test changes are to recover green CI without losing coverage on either harbor version.Documentation
No docs changes needed — the public API (
track_harbor(), span namesetup_environment) is unchanged.