feat: add tags to asset migration

[seanmreidy]

Fixes MDEV-67

Description

We weren't migrating tags as part of datascope migration in the asset_migrator - this change does that.

Testing

Testing details in Linear ticket.

[seanmreidy]

• feat: add tags to asset migration #653 👈 (View in Graphite)
• feat: refactor migration library and introduce statefulness #641
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Greptile Summary

This PR fixes missing series_tags propagation during datascope migration in AssetMigrator. Previously, add_dataset was called without passing the tags from the source data scope; the new implementation fetches the raw DataScope objects via _list_dataset_scopes() to capture series_tags and passes them through to new_asset.add_dataset(...). A few housekeeping refactors accompany the fix: the _copy_optional_runs helper is inlined and the dataset-copy path is now gated behind an explicit dataset_config is not None check.

Key observations:

• Core fix looks correct: source_data_scope.series_tags is properly captured and forwarded to add_dataset, which accepts Mapping[str, str] | None — the types align.
• Dead code: _resolve_destination_dataset (lines 93–110) is no longer called anywhere after its logic was inlined into _copy_asset_datasets; it should be removed.
• Extra API round-trip: _list_dataset_scopes() and list_datasets() each call _get_latest_api() independently, resulting in two asset-fetch requests where one would suffice.
• The behavior change around inaccessible datasets (raising ValueError instead of silently skipping) was raised in a previous review thread and is out of scope here.

Confidence Score: 4/5

• Safe to merge — the core tag-propagation fix is correct and the issues found are minor style/performance concerns.
• The primary functional change (forwarding series_tags via _list_dataset_scopes()) is correctly implemented and the types align with add_dataset's signature. The two flagged issues (dead code and an extra _get_latest_api() round-trip) are non-blocking; they don't affect correctness or data integrity.
• nominal/experimental/migration/migrator/asset_migrator.py — contains dead _resolve_destination_dataset method and a redundant API call in _copy_asset_datasets.

Important Files Changed

Filename	Overview
nominal/experimental/migration/migrator/asset_migrator.py	Adds series_tags propagation when migrating asset datasets; refactors _copy_optional_runs inline and guards _copy_asset_datasets with a dataset_config is not None check. Two minor issues: _resolve_destination_dataset is now dead code, and _list_dataset_scopes() + list_datasets() make redundant back-to-back _get_latest_api() calls.

Sequence Diagram

sequenceDiagram
    participant AM as AssetMigrator
    participant SA as source_asset (Asset)
    participant API as _get_latest_api()
    participant Cat as Catalog API
    participant DM as DatasetMigrator
    participant DA as destination_asset (Asset)

    AM->>SA: _list_dataset_scopes()
    SA->>API: _get_latest_api() [call 1]
    API-->>SA: DataScope[] (with series_tags)
    SA-->>AM: source_data_scopes

    AM->>SA: list_datasets()
    SA->>API: _get_latest_api() [call 2 – redundant]
    API-->>SA: scope RID map
    SA->>Cat: _get_datasets(rids)
    Cat-->>SA: Dataset objects
    SA-->>AM: {dataset_rid: Dataset}

    loop for each DataScope
        AM->>DM: copy_from(source_dataset, DatasetCopyOptions)
        DM-->>AM: new_dataset
        AM->>DA: add_dataset(scope_name, new_dataset, series_tags=source_series_tags)
    end

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: nominal/experimental/migration/migrator/asset_migrator.py
Line: 93-110

Comment:
**`_resolve_destination_dataset` is now dead code**

After the refactor, `_resolve_destination_dataset` is defined but never called anywhere — the logic it previously encapsulated was inlined directly into the new `_copy_asset_datasets` implementation. A quick grep confirms there are no callers of this method anywhere in the repository.

Consider removing the method to avoid confusion for future readers who might wonder why it exists.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: nominal/experimental/migration/migrator/asset_migrator.py
Line: 125-126

Comment:
**Redundant `_get_latest_api()` call**

Both `_list_dataset_scopes()` and `list_datasets()` independently call `_get_latest_api()` internally (which makes a network round-trip to fetch the asset). This means the asset is fetched **twice** in sequence — once to get the `DataScope` objects (for `series_tags`) and once more to get the dataset RID-to-scope mapping.

The two results could be derived from a single `_get_latest_api()` call, e.g. by iterating over `source_asset._list_dataset_scopes()` alone and then calling `_get_datasets()` directly on the collected RIDs, avoiding the extra hop entirely.

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 671c45c}

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 7 additional findings in Devin Review.

[drake-nominal]

are we consistent with this naming scheme? I could have sworn we were doing source_xyz vs. dest_xyz but who knows.

[drake-nominal]

more readable like:

source_datasets = {ds.rid: ds for _, ds in source_asset.list_datasets()}

[drake-nominal]

conditional assignment :)