Skip to content

fix: return created dataset/evaluator from create_version, not bool - #6131

Open
papriwal wants to merge 2 commits into
aws:masterfrom
papriwal:fix/dataset-create-version-return-value-clean
Open

fix: return created dataset/evaluator from create_version, not bool#6131
papriwal wants to merge 2 commits into
aws:masterfrom
papriwal:fix/dataset-create-version-return-value-clean

Conversation

@papriwal

Copy link
Copy Markdown
Collaborator

Callers previously had to call get()/refresh() to retrieve the ARN.

Description

Fixes two issues in the AI Registry create_version API for DataSet and Evaluator:

  1. DataSet.create_version() and Evaluator.create_version() returned a bare bool and silently swallowed exceptions on failure, instead of returning the newly created entity (or raising/returning None on failure). Both now return the created DataSet/Evaluator instance.

  2. Adds description support to Evaluator, matching the existing DataSet pattern. description is now threaded through Evaluator.create(), refresh(), and get(), and AIRHub.import_hub_content() now forwards description to the underlying HubContentDescription API field for both entity types (previously accepted by the AWS API but never exposed by the SDK).

Also fixes a pre-existing bug in Evaluator.__init__'s super().__init__() call: it was missing a positional argument, which caused sagemaker_session to be silently written into the parent class's description slot instead of its own — meaning sagemaker_session was effectively dropped on every Evaluator instance.

Testing

  • Added unit tests covering:
    • create_version returning the created entity (success) and None/raising on failure, for both DataSet and Evaluator.
    • description reaching AIRHub.import_hub_content's call args for both DataSet.create() and Evaluator.create().
    • Evaluator.get() correctly reading back description from the API response.
  • Updated existing integ tests for test_create_dataset_version and test_create_evaluator_version to assert on the returned entity from create_version() instead of a boolean.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

papriwal added 2 commits July 30, 2026 03:10
Callers previously had to call get()/refresh() to retrieve the ARN.
Thread description through Evaluator.create/get/refresh and
AIRHub.import_hub_content, matching DataSet. Also fix a missing
positional arg in Evaluator.__init__ that dropped sagemaker_session
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

Claude finished @papriwal's task in 2m 23s —— View job


Code Review

I reviewed the diff (/tmp/pr.diff) against the base at origin/master. Overall this is a solid, well-tested fix. The super().__init__() correction in Evaluator is a genuine latent bug catch, and returning the created entity is a clear improvement over the old bool. A few points worth considering before merge.

1. Failure semantics diverge between the two sibling methods (main concern)

DataSet.create_version and Evaluator.create_version now handle failure in opposite ways:

  • dataset.py:567-572 — catches the exception, logs it, and returns None.
  • evaluator.py:535-536 — catches and re-raises as RuntimeError.

For two sibling entity types with the same method name and near-identical purpose, this is a surprising inconsistency for SDK consumers. A caller writing generic code over both types can't rely on a single contract: one signals failure with a falsy return, the other with an exception. The DataSet docstring/return type says Optional[DataSet] while Evaluator's says it always returns an Evaluator (and documents Raises: RuntimeError).

Suggest picking one convention for both. Raising is generally the more Pythonic choice (a bare None still "silently swallows" the failure detail the PR set out to fix — the caller only sees None, not why). If you keep DataSet returning None, at least call out the deliberate difference.

Note this is also a subtle backward-incompatibility: existing Evaluator callers doing if not evaluator.create_version(...): previously got False on failure and will now get an exception instead. Probably desirable, but worth being intentional about.

2. Preserve the exception chain (evaluator.py:535-536)

except Exception as e:
    raise RuntimeError(f"[PySDK Error] Failed to create new version: {str(e)}")

Use raise RuntimeError(...) from e so the original traceback/cause is retained. Without from e, the underlying error's context is harder to debug.

3. description isn't carried across versions (minor)

Both create_version implementations call create(...) without passing self.description, so a new version silently drops the description of the version it was derived from. Since this PR is specifically adding description support, it may be worth threading description=self.description through create_version too — otherwise the field is added but not preserved on the most common mutation path.

Nits / positives (no action needed)

  • Good catch on the Evaluator.__init__ super().__init__() positional-arg bug — sagemaker_session was landing in the parent's description slot. Verified against air_hub_entity.py:43-52.
  • Switching from print(...) to a module logger in dataset.py is the right call.
  • Test coverage for the new behavior (success/failure returns, description reaching import_hub_content, Evaluator.get() reading it back) is thorough.

I couldn't post these as inline comments — the inline-comment tool isn't available in this run — so they're consolidated here with file:line references.

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