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

Renaming inner and outer consumes/produces #865

Closed
wants to merge 3 commits into from

Conversation

mrchtr
Copy link
Contributor

@mrchtr mrchtr commented Feb 20, 2024

Propose to rename the inner and outer consumes/produces to something more meaningful.

Idea was to have self explaining names. An OperationSpec is consuming fields from the dataset, and produces fields to dataset (previous outer). The OperationSpec holding operation_consumes, and operation_produces (previous inner). As well as the mapping (operation_to_dataset_mapping and dataset_to_operation_mapping).

@RobbeSneyders
Copy link
Member

Thanks @mrchtr. This doesn't change anything user facing right? I'm wondering if we should make a distinction there as well.

@mrchtr
Copy link
Contributor Author

mrchtr commented Feb 20, 2024

Thanks @mrchtr. This doesn't change anything user facing right? I'm wondering if we should make a distinction there as well.

Indeed, it's only a renaming of methods. It might make sense to define domain names somewhere, as this could likely enhance the explainability of our concepts. For example, a Dataset has a global schema and fields, and Components apply operations on the dataset, etc. .. This could be based on the feedback from the framework blog post, or perhaps we could use the blog post directly for it.

I wouldn't change the pipeline interface. I think we've had this discussion before, where we considered a dataclass for the consumes and produces mapping, and to enable the naming of the dict entries. But it was just more overhead and didn't added so much value.

@mrchtr
Copy link
Contributor Author

mrchtr commented Feb 22, 2024

This PR is outdated. The changes here are already part of #867

@mrchtr mrchtr closed this Feb 22, 2024
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