Skip to content

feat: allow users to set a relative path to save compiled Pipeline DSL#646

Open
Ya-shh wants to merge 5 commits intokubeflow:mainfrom
Ya-shh:feat/configurable-dsl-output-path
Open

feat: allow users to set a relative path to save compiled Pipeline DSL#646
Ya-shh wants to merge 5 commits intokubeflow:mainfrom
Ya-shh:feat/configurable-dsl-output-path

Conversation

@Ya-shh
Copy link
Copy Markdown
Contributor

@Ya-shh Ya-shh commented Feb 28, 2026

By default, Kale saves the generated KFP DSL script to a hidden .kale/ folder, making it hard for users to find.

This PR adds an Output Directory field to the Kale panel in JupyterLab, letting users specify where the compiled pipeline file should be saved (e.g. pipelines/output).

The same option is available via the --output_path CLI flag.

Leaving the field empty preserves the existing .kale/ default.

Closes #630

@google-oss-prow
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ederign for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Ya-shh Ya-shh force-pushed the feat/configurable-dsl-output-path branch from 40b7766 to 5811c54 Compare February 28, 2026 17:38
@Ya-shh
Copy link
Copy Markdown
Contributor Author

Ya-shh commented Feb 28, 2026

Fixes! - #630

@ada333
Copy link
Copy Markdown
Collaborator

ada333 commented Mar 2, 2026

Thank you for working on this @Ya-shh ! The functionality works as expected and the code looks good. Only thing I would change is the error message when putting in invalid path:

image

With that I would also add some invalid paths to the tests. (absolute paths, paths with .. , etc.)

Please let me know if you agree.

@Ya-shh
Copy link
Copy Markdown
Contributor Author

Ya-shh commented Mar 4, 2026

Thank you for working on this @Ya-shh ! The functionality works as expected and the code looks good. Only thing I would change is the error message when putting in invalid path:

image With that I would also add some invalid paths to the tests. (absolute paths, paths with .. , etc.)

Please let me know if you agree.

Hey @ada333 , thanks for the review !
Do you want to update the error 'Message:' description to something more detailed, like:
'/* is not a valid output directory. Please use a relative path'?

@ada333
Copy link
Copy Markdown
Collaborator

ada333 commented Mar 4, 2026

@Ya-shh yes exactly!

Signed-off-by: Yash <yashh.real@gmail.com>
@Ya-shh
Copy link
Copy Markdown
Contributor Author

Ya-shh commented Mar 4, 2026

@Ya-shh yes exactly!

Thanks @ada333 , I have pushed the changes, please take a look

Copy link
Copy Markdown
Collaborator

@ada333 ada333 left a comment

Choose a reason for hiding this comment

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

image

@Ya-shh works as expected, code looks good to me, great that you added tests with the new feature. Thanks!

@Ya-shh
Copy link
Copy Markdown
Contributor Author

Ya-shh commented Mar 13, 2026

I have resolved the merge conflicts

Copy link
Copy Markdown
Collaborator

@jesuino jesuino left a comment

Choose a reason for hiding this comment

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

Thanks again for your contribution. I am suggesting another way to check the path set by the user. Does that make sense? Please also let me know if you think that this could be set using settings - if that's the case we can open an issue later to make it configurable from the client side as well.

Thanks!

def _validate(self, value: str):
if not value:
return
if os.path.isabs(value):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Have you tried to:

  • Resolve the user path using Path.resolve(); Notice that when you call resolve you will have as result the actual path, not symlinks or shortcuts (such as ..)
  • Make it sure that it relative to the target directory, seomthing like: user_path.startswith(project_dir) - this way we ensure that the path is only set relative to the project path and not anywhere else on the disk

Does that make sense to you?

…-output-path

# Conflicts:
#	kale/tests/unit_tests/test_output_path.py
@Ya-shh Ya-shh force-pushed the feat/configurable-dsl-output-path branch from 1ce7658 to 789cae2 Compare March 18, 2026 17:09
@Ya-shh
Copy link
Copy Markdown
Contributor Author

Ya-shh commented Mar 18, 2026

@jesuino Thanks a lot for the review !, I have replaced the string based checks with Path.resolve() + startswith(project_dir) as you suggested. One check now covers absolute paths, .. traversal, and symlinks.

Copy link
Copy Markdown
Collaborator

@jesuino jesuino left a comment

Choose a reason for hiding this comment

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

Hello @Ya-shh

Thanks again for your changes. I did test and it is working perfectly. There's just one point which is the UI for the input path being on the LeftPane. This is better suitable to be a part of Jupyter Lab settings, see this PR by @alikhere . Once yours and theirs PR is merged we should create an issue to move this configuration to the settings.

@google-oss-prow google-oss-prow bot added the lgtm label Mar 23, 2026
@Ya-shh
Copy link
Copy Markdown
Contributor Author

Ya-shh commented Mar 25, 2026

Hello @Ya-shh

Thanks again for your changes. I did test and it is working perfectly. There's just one point which is the UI for the input path being on the LeftPane. This is better suitable to be a part of Jupyter Lab settings, see this PR by @alikhere . Once yours and theirs PR is merged we should create an issue to move this configuration to the settings.

Yes it is better suited there, but personally I find it more useful to have the path directly in the LeftPane, I think it's easier to navigate from there

@jesuino
Copy link
Copy Markdown
Collaborator

jesuino commented Mar 26, 2026

Hello @Ya-shh

I prefer the Settings because I do believe that the Left Pane should have only Pipeline related configuration, but I do understand your point. I asked this on the community, feel free to participate there:

https://cloud-native.slack.com/archives/C08KJBVDH5H/p1774566643127079

@Ya-shh
Copy link
Copy Markdown
Contributor Author

Ya-shh commented Mar 28, 2026

Hey @jesuino in my opinion we should go with Settings as you said .However ,it would be good to make the resolved output path visible somewhere lightweight on the Left Pane, like a small read only label or tooltip near the compile/deploy button (e.g., "Output: .kale/").
That way:

  1. The Left Pane stays clean and pipeline focused
  2. Power users know where to configure it i.e in Settings
  3. Users can still see where output goes without digging

@jesuino
Copy link
Copy Markdown
Collaborator

jesuino commented Mar 30, 2026

Hello @Ya-shh

The settings PR has been merged. Would you please make the changes to support the output path in Settings?

Thanks!

@jesuino
Copy link
Copy Markdown
Collaborator

jesuino commented Apr 9, 2026

Hello @Ya-shh

Just to check if you are still working on this. If you are busy or don't want to work on this anymore please let us know.

Thanks!

@Ya-shh
Copy link
Copy Markdown
Contributor Author

Ya-shh commented Apr 9, 2026

Hey @jesuino, I will work on this. I was quite busy last week, but I have already planned out the code. Please wait, I will try to complete this by the end of the week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[feature] Allow users to set a relative path to save compiled Pipeline DSL

3 participants