Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[dagster-sigma] Standardize translator signature #25432

Merged
merged 5 commits into from
Oct 24, 2024
Merged

[dagster-sigma] Standardize translator signature #25432

merged 5 commits into from
Oct 24, 2024

Conversation

benpankow
Copy link
Member

Summary

Standardizes the DagsterSigmaTranslator format to:

  1. Have a central get_asset_spec, get_asset_key method
  2. Pass the asset_key returned by get_asset_key to get_asset_spec
  3. Enforce that the key returned by get_asset_spec matches the input key

How I Tested These Changes

Updated unit tests

@benpankow benpankow mentioned this pull request Oct 22, 2024
Merged
@benpankow
Copy link
Member Author

benpankow commented Oct 22, 2024 •
edited
Loading

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

Join @benpankow and the rest of your teammates on Graphite Graphite

schrockn
schrockn reviewed Oct 22, 2024
View reviewed changes
python_modules/libraries/dagster-sigma/dagster_sigma/translator.py Outdated
key=asset_key,
metadata=metadata,
kinds={"sigma"},
deps={AssetKey(input_name.lower().split(".")) for input_name in data.inputs},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm shouldn't this mirror logic in get_asset_key

I would have expect a method like asset_key_for_name that is called in both contexts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated impl a bit - in this case, the input_names refer to upstream tables in the user's warehouse which aren't emitted as specs by the integration - they're dot-separated strings my_db.my_schema.my_table. The names of sigma objects in get_asset_key refer to asset specs that the integration is actually outputting, Sigma objects.

schrockn
schrockn previously requested changes Oct 22, 2024
View reviewed changes
Copy link
Member

@schrockn schrockn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Just have the one question about deps case and constructing asset keys from "input name"

@benpankow benpankow mentioned this pull request Oct 22, 2024
Draft
@benpankow benpankow mentioned this pull request Oct 22, 2024
Merged
@benpankow benpankow force-pushed the benpankow/sigma-translator-2 branch 3 times, most recently from c829865 to 6429bfb Compare October 23, 2024 05:59
@graphite-app graphite-app bot added the area: docs Related to documentation in general label Oct 23, 2024
maximearmstrong
maximearmstrong previously requested changes Oct 23, 2024
View reviewed changes
python_modules/libraries/dagster-sigma/dagster_sigma/translator.py Outdated
Comment on lines 83 to 85
def get_asset_spec(
self, asset_key: AssetKey, data: Union[SigmaDataset, SigmaWorkbook]
) -> AssetSpec:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do something similar to PowerBI instead?

I wonder if a function with the signature def get_asset_spec(self, data: Union[SigmaDataset, SigmaWorkbook]) -> AssetSpec would be easier to customize for users. We could call get_asset_key in it instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question - @schrockn suggested that we put the asset_key as a parameter so that it's more clear that there is a contract that the asset key in the spec emitted from get_asset_spec must match the asset key returned by get_asset_key.

The integration itself would call get_asset_key and pass the result into get_asset_spec, e.g.

asset_specs = [get_asset_spec(get_asset_key(obj), obj) for obj in objs]

I don't have a strong preference either way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying.

I'd be in favor of keeping the pattern without the key as a parameter for now, to ensure consistency between integrations.

If we think that passing the key as a parameter to get_asset_spec is better, we should do it for all other translators.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to not pass the key for now

@benpankow
Copy link
Member Author

benpankow commented Oct 24, 2024 •
edited
Loading

Merge activity

  • Oct 24, 1:07 PM PDT: A user started a stack merge that includes this pull request via Graphite.
  • Oct 24, 1:12 PM PDT: Graphite rebased this pull request as part of a merge.
  • Oct 24, 1:13 PM PDT: A user merged this pull request with Graphite.

@benpankow benpankow changed the base branch from benpankow/asdf to graphite-base/25432 October 24, 2024 20:07
@benpankow benpankow changed the base branch from graphite-base/25432 to master October 24, 2024 20:09
@benpankow benpankow merged commit f926daa into master Oct 24, 2024
1 check was pending
@benpankow benpankow deleted the benpankow/sigma-translator-2 branch October 24, 2024 20:13
OwenKephart pushed a commit that referenced this pull request Oct 24, 2024
@benpankow @OwenKephart
[dagster-sigma] Standardize translator signature (#25432)
53fac02 
## Summary

Standardizes the `DagsterSigmaTranslator` format to:

1) Have a central `get_asset_spec`, `get_asset_key` method
2) Pass the `asset_key` returned by `get_asset_key` to `get_asset_spec`
3) Enforce that the key returned by `get_asset_spec` matches the input key

## How I Tested These Changes

Updated unit tests
Grzyblon pushed a commit to Grzyblon/dagster that referenced this pull request Oct 26, 2024
@benpankow @Grzyblon
[dagster-sigma] Standardize translator signature (dagster-io#25432)
f1adb39 
## Summary

Standardizes the `DagsterSigmaTranslator` format to:

1) Have a central `get_asset_spec`, `get_asset_key` method
2) Pass the `asset_key` returned by `get_asset_key` to `get_asset_spec`
3) Enforce that the key returned by `get_asset_spec` matches the input key

## How I Tested These Changes

Updated unit tests
@maximearmstrong maximearmstrong mentioned this pull request Nov 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximearmstrong maximearmstrong maximearmstrong approved these changes

@schrockn schrockn schrockn approved these changes

@OwenKephart OwenKephart Awaiting requested review from OwenKephart

@C00ldudeNoonan C00ldudeNoonan Awaiting requested review from C00ldudeNoonan

@PedramNavid PedramNavid Awaiting requested review from PedramNavid

@cmpadden cmpadden Awaiting requested review from cmpadden

Assignees
No one assigned
Labels
area: docs Related to documentation in general
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benpankow @schrockn @maximearmstrong