[OPIK-6341] [SDK] feat: add --destination-project + --exclude-experiments to opik import dataset#6573
Draft
JetoPistola wants to merge 1 commit intomainfrom
Conversation
…ents to opik import dataset Plumbs `destination_project` through `import_datasets_from_directory`, `import_experiments_from_directory`, `_import_traces_from_projects_directory`, `_build_dataset_item_id_map`, and `recreate_experiments`. Defaults preserve today's `opik import` behaviour exactly. Adds two flags on `opik import dataset`: - `--destination-project NAME` overrides every `project_name` the importers would otherwise infer. - `--exclude-experiments` skips the new sibling-experiments import branch. When `--destination-project` is set and a sibling `experiments/` directory exists alongside `datasets/`, the dataset import path now also runs `import_experiments_from_directory` with the same project override (gated by `--exclude-experiments`). Cross-instance migration paths (no `experiments/` dir) are unchanged. This is the first half of OPIK-6341. The same-workspace suffix-rename workaround (the part that lets self-hosted users import a dataset back into the same workspace under a different project despite the `(workspace_id, name)` uniqueness constraint) is intentionally left for a follow-up commit so the rename design can be reviewed independently. Implements OPIK-6341.
github-actions
Bot
added
python
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
+107
to
+112
| def test_help_lists_new_flags(self) -> None: | ||
| runner = CliRunner() | ||
| result = runner.invoke(cli, ["import", "ws", "dataset", "--help"]) | ||
| assert result.exit_code == 0 | ||
| assert "--destination-project" in result.output | ||
| assert "--exclude-experiments" in result.output |
Contributor
There was a problem hiding this comment.
Test names like test_help_lists_new_flags don’t match the test_<WHAT>__<CASE_DESCRIPTION>__<EXPECTED_RESULT>/test_<WHAT>__happyflow contract—should we rename them to follow the SDK convention?
Finding type: AI Coding Guidelines | 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/unit/cli/test_import_dataset.py around lines 107-512, rename the test
methods in classes TestClickSurface, TestDestinationProjectPlumbing, and
TestSiblingExperimentsBranch to follow the required naming contract from
sdks/python/AGENTS.md and .agents/skills/python-sdk/testing.md (either
`test_<WHAT>__<CASE_DESCRIPTION>__<EXPECTED_RESULT>` or `test_<WHAT>__happyflow`). For
each existing `test_*` method (e.g., test_help_lists_new_flags,
test_destination_project_threaded_into_import_by_type,
test_exclude_experiments_threaded_into_import_by_type,
test_no_flags_preserves_old_invocation, and the sibling experiments/plumbing tests),
update the function name only—keep the test bodies/assertions unchanged. Ensure the
new names clearly encode the WHAT being tested, the scenario/case (flags set, missing
directory, no destination project), and the expected result (called/not called,
destination_project threaded/None, correct project_name usage, exit_code assertions).
Comment on lines
+138
to
+149
| if destination_project is not None and not exclude_experiments: | ||
| experiments_dir = base_path / "experiments" | ||
| if experiments_dir.exists() and any( | ||
| experiments_dir.glob("experiment_*.json") | ||
| ): | ||
| debug_print( | ||
| f"Found experiments directory at {experiments_dir}, importing with destination_project={destination_project!r}", | ||
| debug, | ||
| ) | ||
| experiments_stats = import_experiments_from_directory( | ||
| client, | ||
| experiments_dir, |
Contributor
There was a problem hiding this comment.
This new _import_by_type block duplicates the full experiments import logic in import_all, so should we extract a shared helper (or reuse the existing import_all helper) to keep the experiments import in one place?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Comment on lines
+157
to
+162
| # the summary surfaces both. ``import_experiments_from_directory`` | ||
| # already returns the dataset keys it touched, so prefer | ||
| # the larger of the two when keys overlap rather than | ||
| # silently overwriting dataset counts with zero. | ||
| for key, value in experiments_stats.items(): | ||
| stats[key] = max(stats.get(key, 0), value) |
Contributor
There was a problem hiding this comment.
Using max(...) undercounts the overlapping datasets_*/experiments_*/traces_*/prompts_* counters across the dataset and sibling-experiments phases, so should we accumulate these counters instead of taking the larger value?
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/src/opik/cli/imports/__init__.py around lines 130-163, inside
`_import_by_type` under the `if import_type == "dataset"` and `if destination_project is
not None and not exclude_experiments` block, the code merges `stats` with
`experiments_stats` using `stats[key] = max(stats.get(key, 0), value)`. Replace this
merge strategy so overlapping counters from the dataset import and the
sibling-experiments import are accumulated (e.g., sum them) rather than taking only the
larger value; this prevents the summary and later “Import completed with …
error(s)” logic from undercounting work. Ensure that keys returned only by
`import_experiments_from_directory` (e.g., experiments_*/traces_*/prompts_*) are still
added to `stats` as-is, and only the overlap behavior changes from max to accumulation.
2 tasks
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
First half of OPIK-6341 — adds
--destination-project NAMEand--exclude-experimentstoopik import dataset, and threadsdestination_projectthrough the dataset / experiment importers (transplants the plumbing from parked PR #6555 so it can land independently). With--destination-projectset, the dataset import path now also runsimport_experiments_from_directoryagainst any siblingexperiments/dir using the same project override (gated by--exclude-experiments). Defaults preserve today'sopik importbehaviour exactly: cross-instance migrations and no-flag invocations are unchanged.(workspace_id, name)uniqueness constraint) is not in this commit. Keeping it as a separate follow-up commit so the rename design (<source>__<sanitized_project>,_001/_002fallback,opik-auto-copytag, provenance description) can be reviewed in isolation. Still thinking through edge cases — e.g. how to interact with the planned cleanup-sweep ticket, whether the tag should live on the dataset or as metadata, how to size theget_datasets()collision-detection cap.try get_dataset / except → create_datasetshape rather than switching toget_or_create_dataset(as PR [OPIK-6246] [SDK] feat: add 'opik copy dataset' CLI command #6555 does) to minimize behaviour change in this commit. Commit 2 will likely revisit that choice in the rename branch.Change checklist
Issues
opik copy dataset, parked PR [OPIK-6246] [SDK] feat: add 'opik copy dataset' CLI command #6555 — becomes a thin wrapper once Commit 2 lands)AI-WATERMARK
AI-WATERMARK: yes
localhost:5174(will be done before un-drafting alongside Commit 2)Testing
Unit tests (run from
sdks/python):tests/unit/cli/test_import_dataset.pycovering:--destination-project/--exclude-experimentsparsed and threaded into_import_by_type;--helplists them; no-flags invocation preserves old kwargs (regression guard).destination_projectplumbing:client.get_dataset/client.create_dataset/client.trace/client.spanall receive the override;recreate_experimentsrewritesproject_name; no-override path unchanged.--destination-projectset +experiments/dir present; suppressed by--exclude-experiments; no-op whenexperiments/absent (typicalopik export datasetlayout); does NOT fire without--destination-project.tests/unit/cli/tests still pass — no regressions.Pre-commit (ruff, ruff-format, mypy) clean via
sdks/python/.pre-commit-config.yaml.Manual smoke test against
localhost:5174is pending and will be done before un-drafting alongside Commit 2.Documentation
N/A for this PR. CLI help text is the user-facing surface for v1; broader docs / migration-guide updates will land alongside the wider OPIK-5859 epic.