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

Modify _split_params to handle a list #4205

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

merelcht
Copy link
Member

@merelcht merelcht commented Oct 3, 2024

Description

A client team using Kedro was facing issues with their deployment flow with VertexAI. They managed to workaround most of it, but mentioned their workflow would be simplified if it's possible to pass runtime parameters within a list, because that's how Vertex wraps parameters.

Development notes

Allow parameters to wrapped in a list like --params ["key1=a,key2=b"]

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

@merelcht merelcht marked this pull request as ready for review October 4, 2024 13:32
@merelcht merelcht self-assigned this Oct 4, 2024
@merelcht merelcht requested review from DimedS and noklam October 4, 2024 13:32
Signed-off-by: Merel Theisen <[email protected]>
@noklam
Copy link
Contributor

noklam commented Oct 4, 2024

IndexError: list index out of range
(kedro) gitpod /workspace/project $

I get this error when I do kedro run --params ["key1=a,key2=b"]

@merelcht
Copy link
Member Author

merelcht commented Oct 4, 2024

IndexError: list index out of range
(kedro) gitpod /workspace/project $

I get this error when I do kedro run --params ["key1=a,key2=b"]

You're right.. I think this is annoying click stuff again where things are always wrapped in a string.. I've pushed code to handle this. Can you try again?

Signed-off-by: Merel Theisen <[email protected]>
@noklam
Copy link
Contributor

noklam commented Oct 7, 2024

I am still getting IndexError: list index out of range. Besides, what is the use case for this?

kedro run --params ["key1=a,key2=b"]

Would this be equivalent to kedro run --params key1=a,key2=b?

@merelcht
Copy link
Member Author

merelcht commented Oct 7, 2024

I am still getting IndexError: list index out of range. Besides, what is the use case for this?

kedro run --params ["key1=a,key2=b"]

Would this be equivalent to kedro run --params key1=a,key2=b?

Hmm... 🤔

The use case is in VertexAI it wraps parameters in a list before passing them on. The run command is called slightly different from when it's called through the CLI from what I could see during the walkthrough. I don't know how hard it is to test this properly on VertexAI, but I can ask the client if they want to test it from the branch.

Copy link
Contributor

@ElenaKhaustova ElenaKhaustova left a comment

Choose a reason for hiding this comment

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

LGTM!

@merelcht
Copy link
Member Author

merelcht commented Oct 8, 2024

@noklam I've played around with Vertex, but since the client team has a custom setup it's hard for me to replicate and test it fully. I've contacted them, but they can't install directly from the branch. Would you be okay with merging and releasing this so they can use it more easily? Or do you still have concerns about the solution?

@noklam
Copy link
Contributor

noklam commented Oct 8, 2024

On the testing side:

  • Can they installed from a pre-release version?
  • Or could we publish this as a separate testing so they can play around with this?

On the PR itself, the use case is still a bit mysterious to me. If it's a VertexAI specific thing, why do it need to go in to kedro instead of a plugin, or run.py if it's some customisation for specific project?

From what I have seen in the test, it is different from what get actually run in kedro run that subsequently called _split_params.

For example:
When I run kedro run --params ["key1=a,key2=b"], in _split_params, the value is "[key1=a,key2=b]" instead of ["key1=a,key2=b"] (The new test), so this doesn't feel right as well.

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.

3 participants