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

Detect pipeline attribute during compile/run #398

Merged
merged 6 commits into from
Sep 13, 2023
Merged

Detect pipeline attribute during compile/run #398

merged 6 commits into from
Sep 13, 2023

Conversation

PhilippeMoussalli
Copy link
Contributor

Addresses #375

@PhilippeMoussalli PhilippeMoussalli linked an issue Aug 30, 2023 that may be closed by this pull request
Automatically detect pipeline attribute when running / compiling #375
Closed
@GeorgesLorre
Copy link
Collaborator

GeorgesLorre commented Sep 5, 2023 •
edited
Loading

Nice change ! Makes it easier the import string was indeed a bit confusing.

Some notes:

  1. did you check the documentation for cli examples that might need updating ?
  2. In the original implementation when there was an error in you pipeline code the cli would just say that it could not find your pipeline. There was never any indication (traceback or special message) that there was something wrong, is this easier in this implementation ?

@PhilippeMoussalli
Copy link
Contributor Author

Nice change ! Makes it easier the import string was indeed a bit confusing.

Some notes:

  1. did you check the documentation for cli examples that might need updating ?
  2. In the original implementation when there was an error in you pipeline code the cli would just say that it could not find your pipeline. There was never any indication (traceback or special message) that there was something wrong, is this easier in this implementation ?

Thanks Georges!

  1. Just update the docs.
  2. If there is no pipeline found it will return a valid error. I think the error that you're referring to was because of the default output path (which was wrongly set to None). This shouldn't occur anymore. It's either the default for kfp/docker-compose or a user custom one.

mrchtr
mrchtr approved these changes Sep 13, 2023
View reviewed changes
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 @PhilippeMoussalli, lgtm. I've just found two small typos in the documentation.

docs/getting_started.md
@@ -287,7 +287,7 @@ We add the component to our pipeline definition and specify that it depends on t
We can now easily run our new pipeline:

```bash
fondant run pipeline:my_pipeline --local
fondant run pipeline --local
Copy link
Contributor

Choose a reason for hiding this comment

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

I think one command is outdated here. Either the one in line 142 fondant run pipeline.py --local or this one.

docs/getting_started.md
@@ -139,10 +139,10 @@ Fondant has a feature rich CLI that helps you with these steps. Let's start by r
First of all make sure you have [Docker Compose](https://docs.docker.com/compose/) installed on your system

```bash
fondant run pipeline:my_pipeline --local
fondant run pipeline.py --local
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be fondant compile pipeline --local? Is the .py needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both pipeline and pipeline.py refer to a module and can work with a defined pipeline instance. I added .py here to make it more explicit that we're running an instance defined in a pipeline.py file

@PhilippeMoussalli PhilippeMoussalli merged commit c23189b into main Sep 13, 2023
5 checks passed
@PhilippeMoussalli PhilippeMoussalli deleted the detect-pipeline-attribute branch September 13, 2023 06:59
Hakimovich99 pushed a commit that referenced this pull request Oct 16, 2023
@PhilippeMoussalli
Detect pipeline attribute during compile/run (#398)
2ab9cef 
Addresses #375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgesLorre GeorgesLorre GeorgesLorre approved these changes

@mrchtr mrchtr mrchtr approved these changes

@RobbeSneyders RobbeSneyders Awaiting requested review from RobbeSneyders

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automatically detect pipeline attribute when running / compiling
3 participants
@PhilippeMoussalli @GeorgesLorre @mrchtr