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.

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

[components] Remove translator terminology from SlingReplicationCollectionComponent #26927

Conversation

OwenKephart
Copy link
Contributor

@OwenKephart OwenKephart commented Jan 7, 2025

Summary & Motivation

The translator language was fairly confusing. Went through some iterations but arrived in a place where the translation layer can be overridden in yaml using the asset_attributes field. This field will have the domain-specific object in scope, meaning you can use that to set attributes if desired.

At the same time, this leaves open the ability for users to specify a python-based translator to override the default one.

Made the following changes:

  1. Because we want to use the key asset_attributes to replace translator, this conflicts with the global asset_attributes field, so I renamed this to be asset_transforms. This gives a better sense of the fact that you're doing specific operations over all assets (imo)
  2. Removed the weird subclassing stuff with the existing asset transforms -- this was only necessary in a world where merge / replace supported different argument sets, but this is no longer the case so the extra complexity was unnecessary
  3. Added a generic wrapper layer to make it easier to create these sorts of translator wrappers that incorporate both a base translator as well as jinja templating

How I Tested These Changes

Changelog

NOCHANGELOG

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.

What would the update to the README (in Notion) look like?

e.g. right now we have

type: dagster_components.dbt_project

params:
  dbt:
    project_dir: ../../../dbt/jdbt
  translator:
    key: "target/main/{{ node.name }}"

at one point.

What does it look like in this new regime?

@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from 54fe9ad to e533733 Compare January 8, 2025 17:22
Copy link
Contributor Author

@schrockn

Updated this such that this would end up looking like:

type: dagster_components.dbt_project

params:
  dbt:
    project_dir: ../../../dbt/jdbt
  asset_attributes:
    key: "target/main/{{ node.name }}"

@OwenKephart OwenKephart changed the title [components] Remove translator from SlingReplicationCollectionComponent [components] Remove translator terminology from SlingReplicationCollectionComponent Jan 8, 2025
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from e533733 to 65347e4 Compare January 8, 2025 17:29
@OwenKephart OwenKephart force-pushed the 01-07-_components_convert_params_schema_from_classvar_to_classmethod branch from 22c7cdb to f232896 Compare January 8, 2025 17:39
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from 65347e4 to 2402ae9 Compare January 8, 2025 17:39
@OwenKephart OwenKephart marked this pull request as ready for review January 8, 2025 20:29
@OwenKephart OwenKephart requested review from schrockn and removed request for schrockn January 8, 2025 20:29
@OwenKephart OwenKephart force-pushed the 01-07-_components_convert_params_schema_from_classvar_to_classmethod branch from f232896 to a615bdd Compare January 8, 2025 20:42
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch 2 times, most recently from e556feb to f62635e Compare January 8, 2025 21:53
@OwenKephart OwenKephart force-pushed the 01-07-_components_convert_params_schema_from_classvar_to_classmethod branch from a615bdd to 9dbf553 Compare January 8, 2025 22:27
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from f62635e to 386ea31 Compare January 8, 2025 22:27
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.

Should it just be "transforms" since it might select asset checks only?

Copy link
Contributor Author

sounds good to me

@OwenKephart OwenKephart dismissed schrockn’s stale review January 9, 2025 23:07

updated to transforms!

@OwenKephart OwenKephart requested a review from schrockn January 9, 2025 23:07
@OwenKephart OwenKephart force-pushed the 01-07-_components_convert_params_schema_from_classvar_to_classmethod branch from 9dbf553 to aafd7b9 Compare January 9, 2025 23:14
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from 386ea31 to 4caa07f Compare January 9, 2025 23:14


def get_wrapped_translator_class(translator_type: type):
"""Temporary hack to allow wrapping of many methods of a given translator class."""
Copy link
Member

Choose a reason for hiding this comment

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

Why is it temporary? What is the "permanent" fix?

Copy link
Member

Choose a reason for hiding this comment

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

This does strike me as wacky and something that could end up living for a long time.

I would expect a more "normal" adapter class per component that wraps as instance of the original.

The "heavy lifiting" in this class is _get_rendered_attribute which could be called as a standalone function

@dataclass
class RenderingInfo:
  obj_name: str
  asset_attributes: AssetAttributesModel,
  value_renderer: TemplatedValueRenderer

  def get_rendered_attr(...): ...

   def get_asset_key(self, obj, default) -> AssetKey:
     return self.get_rendered_attr("key", obj, default)

class RenderingSlingTranslator(SlingTranslator)
   rendering_info: RenderingInfo
   wrapped_translator: SlingTranslator

   def get_asset_key(self, obj) -> AssetKey:
    return self.rendering_info.get_asset_key(obj, self.wrapper_translator) 

Something like this is a little more code but I think much more obvious

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.

dynamically created class seems too magical

@OwenKephart OwenKephart force-pushed the 01-07-_components_convert_params_schema_from_classvar_to_classmethod branch from aafd7b9 to 28c32aa Compare January 10, 2025 21:45
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from 4caa07f to 25b8727 Compare January 10, 2025 21:45
@OwenKephart OwenKephart dismissed schrockn’s stale review January 10, 2025 21:47

added explanation, refactored the implementation to be "ready for the future"

@OwenKephart OwenKephart force-pushed the 01-07-_components_convert_params_schema_from_classvar_to_classmethod branch from 28c32aa to 726490d Compare January 10, 2025 22:43
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from 25b8727 to eb16a9d Compare January 10, 2025 22:44
@OwenKephart OwenKephart requested a review from schrockn January 10, 2025 22:55
@OwenKephart OwenKephart force-pushed the 01-07-_components_convert_params_schema_from_classvar_to_classmethod branch from 726490d to 8d1f82c Compare January 10, 2025 22:57
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from eb16a9d to 7be98c0 Compare January 10, 2025 22:58
Copy link
Contributor Author

OwenKephart commented Jan 11, 2025

Merge activity

  • Jan 10, 8:20 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Jan 10, 8:25 PM EST: Graphite rebased this pull request as part of a merge.
  • Jan 10, 8:26 PM EST: A user merged this pull request with Graphite.

@OwenKephart OwenKephart changed the base branch from 01-07-_components_convert_params_schema_from_classvar_to_classmethod to graphite-base/26927 January 11, 2025 01:20
@OwenKephart OwenKephart changed the base branch from graphite-base/26927 to master January 11, 2025 01:22
@OwenKephart OwenKephart force-pushed the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch from 7be98c0 to bba23e4 Compare January 11, 2025 01:24
@OwenKephart OwenKephart merged commit 9229d33 into master Jan 11, 2025
1 check was pending
@OwenKephart OwenKephart deleted the 01-07-_components_remove_translator_from_slingreplicationcollectioncomponent branch January 11, 2025 01:26
marijncv pushed a commit to marijncv/dagster that referenced this pull request Jan 21, 2025
…ctionComponent (dagster-io#26927)

## Summary & Motivation

The translator language was fairly confusing. Went through some iterations but arrived in a place where the translation layer can be overridden in yaml using the `asset_attributes` field. This field will have the domain-specific object in scope, meaning you can use that to set attributes if desired.

At the same time, this leaves open the ability for users to specify a python-based translator to override the default one.

Made the following changes:

1. Because we want to use the key `asset_attributes` to replace `translator`, this conflicts with the global `asset_attributes` field, so I renamed this to be `asset_transforms`. This gives a better sense of the fact that you're doing specific operations over all assets (imo)
2. Removed the weird subclassing stuff with the existing asset transforms -- this was only necessary in a world where merge / replace supported different argument sets, but this is no longer the case so the extra complexity was unnecessary
3. Added a generic wrapper layer to make it easier to create these sorts of translator wrappers that incorporate both a base translator as well as jinja templating

## How I Tested These Changes

## Changelog

NOCHANGELOG
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