add other supported jobspec fields#6
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9c059a3cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| name = job_spec.name | ||
| executable = job_spec.executable | ||
| argv = job_spec.arguments or None |
There was a problem hiding this comment.
submit_job now unconditionally reads job_spec.arguments, but submit_job_script still passes a minimal object without that attribute, so script submissions will raise AttributeError before reaching Slurm. This is a regression for the /compute/job/script path and affects any caller that provides a non-Pydantic or partial spec object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
we are removing job/script right?
| if not job_spec.resources.exclusive_node_use: | ||
| exclusive = ["false"] | ||
| if job_spec.resources.memory: | ||
| memory_mb = max(1, job_spec.resources.memory // (1024 * 1024)) |
There was a problem hiding this comment.
resources.memory is defined in bytes, but converting with floor division to MB can request less memory than the user asked for (e.g., any non-MiB-aligned value is truncated). That under-allocation can lead to avoidable OOM failures; this conversion should round up so the scheduler request is never below the requested byte count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
okay, I so I guess it is saying divide by 1000^2 ?
note, I am not too sure about this, it will have to be tested…
swelborn
force-pushed
the
passthru-custom-attr
branch
from
b65ad7e to
55b4cac
Compare
|
Tested this in a local dev setup to ensure it passes the current compute tests |
add other supported jobspec fields
2 participants
i committed these one by one, so we could remove some of they are not really supported on our system
this exposes more of the jobspec to us.
do not merge yet, this will probably require some testing...
TODO: we still don’t have pre_launch, post_launch, container, and launcher supported here
This is part 2 of 2 in a stack made with GitButler: