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

Feature/dataset based compilers #917

Conversation

GeorgesLorre
Copy link
Collaborator

No description provided.

@GeorgesLorre GeorgesLorre force-pushed the feature/dataset-based-compilers branch from ec1c8b6 to fef8653 Compare April 2, 2024 09:07
@GeorgesLorre GeorgesLorre force-pushed the feature/dataset-based-compilers branch from fef8653 to 922ab4e Compare April 2, 2024 21:46
Copy link
Contributor

@mrchtr mrchtr left a comment

Choose a reason for hiding this comment

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

Thanks @GeorgesLorre!

@@ -857,4 +858,4 @@ def write(
resources=resources,
cache=cache,
)
self._apply(operation)
return self._apply(operation)
Copy link
Contributor

Choose a reason for hiding this comment

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

The only difference to the .apply method is that the write method does ignore the produces.

I would leave it like this but what do you think about adding a deprecation warning to the write method?

We could use this method in the future export the dataset from the working directory to a remote storage for instance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I completely understand. Are we deprecating the write ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats the question if we want to do it. The apply and write looking quite similar now.

@GeorgesLorre GeorgesLorre marked this pull request as ready for review April 3, 2024 13:14
Copy link
Contributor

@mrchtr mrchtr left a comment

Choose a reason for hiding this comment

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

Thanks @GeorgesLorre! I like the workflow wording instead of pipeline in the documentation. Maybe we can use this more often.

We could explain these concepts somewhere else.
E.g.: A dataset can be materialized by running a workflow. The workflow has to be compiled first.

docs/runners/local.md Show resolved Hide resolved
docs/runners/sagemaker.md Show resolved Hide resolved
docs/runners/sagemaker.md Outdated Show resolved Hide resolved
docs/runners/sagemaker.md Outdated Show resolved Hide resolved
docs/runners/sagemaker.md Outdated Show resolved Hide resolved
docs/runners/vertex.md Outdated Show resolved Hide resolved
docs/runners/vertex.md Outdated Show resolved Hide resolved
docs/runners/kfp.md Outdated Show resolved Hide resolved
@GeorgesLorre
Copy link
Collaborator Author

@mrchtr I updated the docs based on your suggestions

@GeorgesLorre GeorgesLorre merged commit 4ac7a0b into feature/refactore-pipeline-interface Apr 4, 2024
8 checks passed
@GeorgesLorre GeorgesLorre deleted the feature/dataset-based-compilers branch April 4, 2024 09:04
GeorgesLorre added a commit that referenced this pull request Apr 4, 2024
mrchtr added a commit that referenced this pull request Apr 5, 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