-
Notifications
You must be signed in to change notification settings - Fork 17
feat(rfc): Write RFC 004 to propose a grab bag of extensions #95
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
base: mainline
Are you sure you want to change the base?
Conversation
Signed-off-by: Cody Edwards <[email protected]> added stuff again Signed-off-by: Cody Edwards <[email protected]> updated more stuff did more feat(rfc): Write RFC 004 to propose a grab bag of extensions Signed-off-by: Cody Edwards <[email protected]>
|
|
||
| ```yaml | ||
| specificationVersion: jobtemplate-2023-09 | ||
| name: LONGPROJECTNAME_EPISODE23_SEQUENCE54_SHOT23_REV12_BACKGROUND_CAMERA001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love a better example for this.
| args: [ <ArgString>, ... ] # @optional @fmtstring[host] | ||
| timeout: <posinteger> # @optional | ||
| cancelation: <CancelationMethod> # @optional | ||
| +python | bash | cmd | powershell | node: <DataString> # @optional @fmtstring[host] @extension FEATURE_BUNDLE_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would love feedback on this notation, I'm not sold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Based on the below this option is mutually exclusive with command but nothing else. Can we indicate that in some way here?
- What would the recommended way a user specifies a version of these tools? Environment set-up?
- maybe we should sort the options here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Open to suggestions for notation here. We could probably add new notation for mutual exclusion.
-
This is the wording we have for command currently
A Format String containing the name of a runnable command that is run on a Worker host.
I can be more explicit here and say "X is expected to be in the system PATH"
-
👍
| ```yaml | ||
| name: <AmountCapabilityName> | ||
| min: <nonnegativefloat> # @optional | ||
| +min: <nonnegativefloat> | <nonnegativefloatstring> # @optional @fmtstring @extension FEATURE_BUNDLE_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens a few times in this RFC. We need a good way of saying this type union is only valid with the extension. Not sold on this notation and would love suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this probably works for our use-case here. maybe if we modify the same fields again in another extension (before releasing a new spec revision) then we'll have to do something else.
| 1. **Automated Naming**: When a template is generated programmatically, it's | ||
| natural to concatenate identifiers from the input to get a unique name. The | ||
| current length limits prevent that from working naturally. Identifier names can | ||
| similarly be generated by concatenating identifiers, so we likely want to treat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where you say "we likely want", you need to make a choice so the proposal is concrete. Either the RFC is making that change too, or it's not.
| 4. **Quicker Iteration**: Further parameterization allows for quicker iteration | ||
| times. During experimentation it is quicker to modify a parameter and resubmit a | ||
| job than to edit the source file and resubmit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values can also depend on the scene or other job inputs, they're not necessarily fixed for a particular job template. Because of that, it's not just for experimentation, it also matters in production.
| 1. Allowable characters: Any unicode character except those in the Cc unicode character category. | ||
| 2. Minimum length: 1 characters. | ||
| -3. Maximum length: 64 characters. | ||
| +3. Maximum length: 512 characters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should say that it's 64 characters unless the FEATURE_BUNDLE_1 extension is enabled and is then 512. Similarly for the other length changes. It would change to just be 512 in the next dated release of the spec, where the extensions become part of the main profile.
| sequential actions in the `onRun` of a task, with each command having separate | ||
| timeout and cancelation behavior. | ||
|
|
||
| This feature was rejected for inclusion in this RFC because it introduces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the idea is rejected from the RFC, not in general, I'd suggest to not mention it at all, and rather create a new discussion topic dedicated to it and comment on the existing one that this point is moved there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to this discussion. Will remove this section in the next revision.
| 4. *cancelation* — If defined, provides details regarding how this action should be canceled. If not provided, then it is | ||
| treated as though provided with `<CancelationMethodTerminate>`. | ||
|
|
||
| +5. *python | bash | cmd | powershell | node* - Syntactic sugar for scripts, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like these
| 3. **Simplified Template Authoring**: Syntax sugar for common interpreters | ||
| reduces boilerplate and makes templates more readable and maintainable. | ||
| 4. **Quicker Iteration**: Further parameterization allows for quicker iteration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first read I didn't quite catch which of the changes fall under this - the parametrized action timeout and notifyPeriodInSeconds are described up top under "Enhanced format string support for" but being able to easily resubmit with parameter overrides for these is I think the real motivation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks! Maybe worth rephrasing in the summary to make that clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
| 1. Allowed characters: Any unicode character except those in the Cc unicode character category. | ||
| 2. Minimum length: 1 character. | ||
| -3. Maximum length: 128 characters, after the format string has been resolved. | ||
| +3. Maximum length: 512 characters, after the format string has been resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of the other names follow the "after the format string has been resolved" wording even if it's true. Maybe we need to fix the spec to make it consistent wording (no behaviour change)?
| ```yaml | ||
| name: <AmountCapabilityName> | ||
| min: <nonnegativefloat> # @optional | ||
| +min: <nonnegativefloat> | <nonnegativefloatstring> # @optional @fmtstring @extension FEATURE_BUNDLE_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this probably works for our use-case here. maybe if we modify the same fields again in another extension (before releasing a new spec revision) then we'll have to do something else.
| 2. Must start with a letter or underscore character. | ||
| 3. Minimum length: 1 character | ||
| -4. Maximum length: 64 character | ||
| +4. Maximum length: 512 character |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: characters. Probably just an issue in the base spec
| args: [ <ArgString>, ... ] # @optional @fmtstring[host] | ||
| timeout: <posinteger> # @optional | ||
| cancelation: <CancelationMethod> # @optional | ||
| +python | bash | cmd | powershell | node: <DataString> # @optional @fmtstring[host] @extension FEATURE_BUNDLE_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Based on the below this option is mutually exclusive with command but nothing else. Can we indicate that in some way here?
- What would the recommended way a user specifies a version of these tools? Environment set-up?
- maybe we should sort the options here
| - | ||
| + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accidental inclusion? or what's changed
| + 2. *bash* - Implicitly creates a Bash embedded file, | ||
| + `command` becomes `bash`, and `args` get prefixed with the implicitly | ||
| + generated file. The file extension is `.sh`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it also be runnable? I guess only if it can be referenced outside this command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want it to be runnable. As it's implicitly run with bash, so there's no need for the executable permission to be set. It seems to me that making the file runnable is something that should be explicit? I could be off base there though.
| + 1. *python* - Implicitly creates a Python embedded file, | ||
| + `command` becomes `python`, and `args` get prefixed with the implicitly | ||
| + generated file. The file extension is `.py`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, do we need a way to refer to that embedded file elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems that this sort of depends on if we want to be able to refer to the same embedded file in an environment OnEnter/OnExit, since currently there is only OnRun for task runs. Maybe there's an an implicit embedded file we can expose via the action or top-level name?
Otherwise that's a use-case where maybe we acknowledge that the syntactic sugar doesn't make sense.
| + 1. *python* - Implicitly creates a Python embedded file, | ||
| + `command` becomes `python`, and `args` get prefixed with the implicitly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering about an implementation detail specific to the python one. Some Linux distributions Python installs do not provide a python command on the PATH. They now only provide python3.
For example, on Ubuntu we see:
$ uname -a
Linux ip-172-31-11-201 6.14.0-1011-aws #11~24.04.1-Ubuntu SMP Fri Aug 1 02:07:25 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
$ python
sh: 2: python: not found
$ python3
Python 3.12.3 (main, Jun 18 2025, 17:59:45) [GCC 13.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>>We have a couple of options here:
- Specify that OpenJD-compatible session implementations account for this (determine whether
python/python3is available, and use it) - Provide a
pythonandpython3option here to let OpenJD template authors specify the command - Leave this as-is, but maybe expand in the spec that worker hosts have the
pythoncommand available. We could maybe document that debian-based distros can use thepython3-is-pythonpackage to achieve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also the converse issue, when there is python but not python3. I think it's important to nudge towards portable job templates, and solutions that avoid using a python3 command are better for portability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can keep it as is and add more clarification in the description that states something like
python (all lowercase) is expected to be a runnable command on a Worker host.
and similar for all other interpreters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe that messaging adjusted with "available to the user running the command". It's definitely something folks can get tripped up on for PATH, aliases, etc. if they only modify their user and now who runs the command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it more "available in the runtime environment" compared to "available to the user"? The command could be provided by a step environment, a job environment, an externally defined environment, the user PATH configuration, or the system PATH configuration.
Signed-off-by: Cody Edwards <[email protected]>
| * *data* — The string data that will be written to the file exactly as it appears. | ||
| * *endOfLine* — The line endings that the embedded file will have when written | ||
| to disk. If `AUTO` the embedded file will have the default line endings of the | ||
| host operating system. Default is `AUTO`. Requires the `FEATURE_BUNDLE_1` extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these two lines need to be indented to continue the bullet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess this formats well in the rendered view, it's just a question of how it looks as a text file. Not important to change.
| 1. *specificationVersion* — A literal that identifies the document as adhering to this schema. | ||
| 2. *$schema* — Ignored. This property is allowed for compatibility with JSON-editing IDEs. | ||
| 3. *extensions* — If provided, is a non-empty list of extensions to the schema. Introduced in [RFC 0002](https://github.com/OpenJobDescription/openjd-specifications/blob/mainline/rfcs/0002-model-extensions.md). | ||
| * Extensions available for specification version 2023-09: [TASK_CHUNKING](https://github.com/OpenJobDescription/openjd-specifications/blob/mainline/rfcs/0001-task-chunking.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll want to rebase to include b9a0eda in your history, then add the extension name to this list.
Tracking Issue: #92
This is a request for comments about Collection of Small Improvements.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.