Fix custom training scripts that define train()#23
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/ok-to-test |
|
Hey @Kaladin-Stormborn, can you sign off your commit here ⬆️ |
Signed-off-by: Kaladin-Stormborn <Kaladin-Stormborn@users.noreply.github.com>
caf4692 to
c68db52
Compare
|
Thanks @Kaladin-Stormborn for this fix 🚀 |
| return ns[func_name] | ||
|
|
||
|
|
||
| def _uncalled_train_call(script: str, func_args: dict[str, Any] | None = None) -> str | None: |
There was a problem hiding this comment.
Nit (non-blocking): thinking can we consider treating top-level async def train() like def train() for the append path (ast.AsyncFunctionDef). If we append a call, it likely needs await train(...) inside the generated sync wrapper (or document that only sync def train() is supported for now).
PR Draft: #19
Title
Fix custom training scripts that define an uncalled
train()Summary
This fixes
run_custom_trainingscript wrapping when a user provides a script that already defines a top-leveltrain()function but does not call it.Previously
_make_train_func()always wrapped the submitted script inside another generateddef train():. For scripts like:the user-defined
train()became a nested function and was never invoked when the generated wrapper ran.The patch detects a top-level
def train()with no module-leveltrain()call and appends a call inside the generated wrapper. Scripts that already calltrain()are left alone to avoid double execution. Matchingfunc_argsare forwarded to a user-definedtrain(...)signature.Tests
Results:
Current diff size:
59insertions,1deletion.84insertions.143insertions,1deletion.Changed Files
kubeflow_mcp/trainer/api/training.pykubeflow_mcp/trainer/api/sdk_contracts_test.py