[puwun]

What this PR does / why we need it:
create_job() skips adding the kueue.x-k8s.io/queue-name label when the specified local-queue doesn't exist, preventing Kueue from ever managing the job. Removed the conditional gate so the label is always added when queue_name is provided.

Changes:

• Removed the if queue_exists is not False condition that gated label addition, _check_queue_exists() is still called for its side effects (warning logs, error handling)
• Updated the "valid flow with non-existent queue" test case to expect the queue label

Which issue(s) this PR fixes:
Fixes #2775

[github-actions]

🎉 Welcome to the Kubeflow Trainer! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

• If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
• Our team will review your PR soon! cc @kubeflow/kubeflow-trainer-team

Join the community:

• Slack: Join our #kubeflow-trainer Slack channel.
• Meetings: Attend the Kubeflow AutoML and Training Working Group bi-weekly meetings.

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

[jiayuzhao05]

suggestions: consider guarding against empty queue names (e.g. if queue_name and queue_name.strip():) to avoid writing an empty queue-name label.

[kannon92]

/ok-to-test

[astefanutti]

@puwun thanks!

@abhijeet-dhumal do you remember if there is a reason why we decided to skip the Kueue label in that case when you worked on #2636?

[abhijeet-dhumal]

@puwun thanks!

@abhijeet-dhumal do you remember if there is a reason why we decided to skip the Kueue label in that case when you worked on #2636?

@astefanutti Actually the original logic in #2636 was designed around a "fail-safe" - if we couldn't confirm the queue exists (404), we'd skip the label to avoid the job being stuck waiting for a non-existent queue.

But this was wrong thinking:

1. Race condition: The queue might be created after the check but before/after job creation - Kueue would then manage it correctly
2. User intent: If user explicitly passes queue_name, they want Kueue management. Silently skipping the label violates that intent.
3. Kueue behavior: Jobs without the label won't be managed by Kueue at all, which seems bad than being queued for a missing LocalQueue (which at least surfaces clearly in Kueue status)

This is the context IIRC, But would be happy to discuss it further whether this decision decision seems correct?

[abhijeet-dhumal]

@puwun LGTM with one nit - Add the empty string guard as suggested by @jiayuzhao05.
The fix is correct. The original edge case table in #2636 had:

queue_name only, 404: No label, warning, job created.

This was a mistake - the label should always be added when the user explicitly requests queue management.

[astefanutti]

@abhijeet-dhumal thanks for the feedback.

@puwun can you please handle the empty string case as suggested by @jiayuzhao05 so it can be merged.

[puwun]

Thanks all, updated with the changes 👍🏼

[astefanutti]

Thanks @puwun!

/lgtm
/approve

[andreyvelich]

/lgtm
/approve

[google-oss-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andreyvelich, astefanutti

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• sdk/python/OWNERS [andreyvelich]

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

[andreyvelich]

Will manually merge it.