WIP: Fix Vertex AI scheduler 64-character limit for long pipeline names (#2262)#5080
WIP: Fix Vertex AI scheduler 64-character limit for long pipeline names (#2262)#5080hrushi2602-cyber wants to merge 6 commits into
Conversation
Co-authored-by: ZenML GmbH <info@zenml.io> (cherry picked from commit 2de41ef)
* Updated docs to include K8s distros * Adjusted for accuracy and clarity * Also added to pro section
20e1366 to
6b9bd79
Compare
schustmi
left a comment
There was a problem hiding this comment.
Hey @hrushi2602-cyber, thanks for the PR! Can you please rebase your changes on the develop branch and also develop as a base branch for your PR.
Regarding the implementation:
We already have something very similar to this implemented in the get_orchestrator_run_name(...) function (see src/zenml/orchestrators/utils.py). In my opinion, it would be nicest if we do the following:
- Add a new function in the
string_utilsmodule. This should be generic, receives as input some original string, a optional maximum length, a suffix length and maybe also a separator string (e.g. "-" or "_") . It should then generate a random suffix, truncate the original string so a suffix of that length fits and then merge them. - We call this function both from
get_orchestrator_run_name(...)as well as the one you introduced (sanitize_vertex_job_name(...))
6b9bd79 to
8f98065
Compare
|
Thanks for the feedback! I've updated the PR based on your suggestions. Here is a summary of the changes: Rebased and Targeted develop: I have successfully changed the PR base branch to develop and rebased my local commits on top of the latest changes from the upstream develop branch. Centralized String Utility: Added a new generic function append_random_suffix in src/zenml/utils/string_utils.py. It handles suffix generation, strict length limit enforcement, and safe string merging. I also included a charset argument to ensure backward compatibility with existing components that might expect hexadecimal strings. Refactored get_orchestrator_run_name: Updated src/zenml/orchestrators/utils.py to offload the string truncation and merging logic to the new utility. The original logic that dynamically shrinks the suffix (while enforcing the minimum of 8 characters) and uses the hexadecimal charset was fully preserved so this won't break downstream orchestrators. Refactored sanitize_vertex_job_name: This function now acts as a pre-filter to enforce Vertex-specific rules (lowercase, alphanumeric, starting with a letter, replacing underscores with hyphens), and then defers to append_random_suffix to safely enforce the 64-character limit. |
schustmi
left a comment
There was a problem hiding this comment.
Few minor things, other than that looks good to me
| original_string=pipeline_name, | ||
| suffix_length=suffix_length, | ||
| max_length=max_length, | ||
| separator="_", |
There was a problem hiding this comment.
I think we should also use separator="" here for backwards compatibility. All orchestrators use this function, and they might not allow underscores.
| """ | ||
| Appends a random suffix to a string, truncating the original if necessary. | ||
| """ | ||
| # Use the provided charset instead of hardcoding alphanumeric |
There was a problem hiding this comment.
| # Use the provided charset instead of hardcoding alphanumeric |
| separator: str = "-", | ||
| charset: str = string.ascii_lowercase + string.digits | ||
| ) -> str: | ||
| """ |
There was a problem hiding this comment.
Please adjust these to follow the google docstring format.
| suffix_length=suffix_length, | ||
| max_length=max_length, | ||
| separator="_", | ||
| charset="0123456789abcdef" # <--- Ensures 100% backward compatibility |
There was a problem hiding this comment.
| charset="0123456789abcdef" # <--- Ensures 100% backward compatibility | |
| charset="0123456789abcdef" |
|
All requested styling changes have been made! Here is what was updated in the latest commits: Google Docstrings: Updated the append_random_suffix utility to strictly follow the Google docstring format. Linter Fixes: Swapped single quotes to double quotes, removed the duplicate re import, and properly sorted the import block in vertex_orchestrator.py. I ran the ruff checks locally on the files changed by me and everything is passing clean with Ruff now. Let me know if you need anything else. |
|
@schustmi Just a gentle reminder! I just applied your suggested changes just check once and let me know... Just commenting in case it got burried under due to the weekend!! |
Description
Relates to Issue: #2262
The Problem
When submitting a pipeline with a name longer than 64 characters to the Vertex AI orchestrator, the API rejects the payload due to strict
display_namecharacter limits. Simply slicing the string at 64 characters risks name collisions if multiple pipelines share a long prefix.The Solution
Introduced a local helper function,
sanitize_vertex_job_name, insidevertex_orchestrator.pythat intercepts bothorchestrator_run_nameandpipeline_name.display_nameis exactly 64 characters and guaranteed unique to avoid overwriting jobs.Current Status (WIP)
I am opening this as a Draft PR to show intent and verify this hashing approach aligns with the maintainers' expectations before wiring up the full test coverage.
Looking forward to any feedback!