refactor: migrate docker & wasm run to v2 api #3970
Conversation
|
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
cmd/cli/docker/docker_run.go
Outdated
| labels, err := parse.Labels(ctx, opts.SpecSettings.Labels) | ||
| if err != nil { | ||
| return nil, err | ||
| // TODO(forrest) [refactor]: this logic is duplicated in wasm_run |
There was a problem hiding this comment.
I should extract the common logic here before merging.
cmd/cli/docker/docker_run.go
Outdated
| ), | ||
| ) | ||
| task, err := models.NewTaskBuilder(). | ||
| Name("TODO"). |
There was a problem hiding this comment.
We currently don't have a means for users to specify a task name with the current flags on wasm run or docker run I could stub in a UUID here and add a --task-name flag later, thoughts?
There was a problem hiding this comment.
we can use "main" as the default task name. Task names are only useful when we support multiple tasks per job, and that won't be doable through docker run and wasm run as they will most likely submit a single task per job. So calling it "main" or any other standard name is acceptable
actually this is what we are currently doing
cmd/cli/docker/docker_run.go
Outdated
| Name: "TODO", | ||
| Namespace: "TODO", |
There was a problem hiding this comment.
Similar point to task name in my comment above - these could be UUIDs, values from flags we add, or something else?
There was a problem hiding this comment.
we have a default namespace for bacalhau job run
bacalhau/pkg/models/constants.go
Line 5 in 6f00c8a
though we are still using clientID for docker run. I think we should use the default namespace, but expose a flag to set the desired namespace. This means users won't be able to filter their own jobs in the public network, but that might be acceptable based on our longer term plans. Thoughts?
cmd/cli/docker/docker_run.go
Outdated
| Type: models.JobTypeBatch, | ||
| Priority: 0, |
There was a problem hiding this comment.
I believe it is assumed that docker and wasm jobs are batch when run over the CLI, but we could easily change that. Additionally priority doesn't have a flag. Gut says we need flags for these - ideas on good default values appreciated.
There was a problem hiding this comment.
there is a --target flag that controls this:
--target all|any Whether to target the minimum number of matching nodes ("any") (default) or all matching nodes ("all") (default any)
any is a batch job, where all is an ops job
There was a problem hiding this comment.
My comment here was with respect to the priority fields. Are we okay adding a flag called --priority that sets the field?
With respect to the job type, currently controlled via --target:
Would we be willing to drop the target flag in favor of a --type flag whose default value is batch and other possible values are service, ops, and daemon? This would bring the CLI more inline with the fields of the job spec.
There was a problem hiding this comment.
lets do the migration one step at a time. Lets avoid exposing or renaming flags as this PR's main focus should be internal and to just switch to new APIs.
To answer your questions though, --priority is not fully implemented as the compute nodes for example don't respect priority when queueing executions locally in their buffer. Will make sense to expose priority, blog about it and make sure it is documented when it is fully implemented. We should use whatever default value bacalhau job run has.
As for --target, we don't and should not support service or daemon jobs using imperative job submissions (i.e. docker run and wasm run). We should allow users to easily update their long running jobs, compare what is deployed in the network and what their local job spec says, and utilize gitops to deploy and update jobs based on committed specs. All that is not feasible with imperative job submissions, and we shouldn't make it easy for users to do bad things to their network.
cmd/util/flags/cliflags/spec.go
Outdated
| `name:path of the output data volumes. `+ | ||
| `'outputs:/outputs' is always added unless '/outputs' is mapped to a different name.`, | ||
| `name=path of the output data volumes. `+ | ||
| `'outputs=/outputs' is always added unless '/outputs' is mapped to a different name.`, |
There was a problem hiding this comment.
I don't think this is true anymore. Will need to address this before merging.
There was a problem hiding this comment.
why changed from name:path to name=path? this will break users. Also we will need to define a default output path outputs:/outputs for both docker run and wasm run to avoid breaking existing behavour and breaking all our examples and docs. I don't think it is a bad design anyways for those commands, but worth exploring the right long term option with @aronchick
There was a problem hiding this comment.
For context here @aronchick: This comment is about a change in behavior of the output flag from expecting : as a separator to = as a separator.
When using a flag that requires pairs of strings (key,value) - for example environment variables - and in the case here ResultPaths I find a StringToStringVar flag type (a map) to provide clearer validation compared to accepting a StringArrayVar flag (a slice). As an example, consider:
Valid:
bacalhau docker run -o outputs=/outputs -o inputs=/inputs
Invalid:
bacalhau docker run -o outputs=/outputs -o inputs
Cobra, our CLI library, will immediately catch the invalid case and return an error since inputs is missing a value field.
Comparatively, the behavior prior to this change accepted both options, and it was on us (developers) to write the validation logic to catch the invalid case of a key missing a value.
If we feel this is too large of a change for our users to understand - replacing : with = - then I can write a bespoke implementation of a map flag parser that uses : as the separator instead of = to preserve existing behavior.
There was a problem hiding this comment.
Can we please stop changing behaviour or adding more complexity to the migration story? The migration of the APIs and CLIs is a huge and complex story as is, and we've discussed that no change in behaviour to be done while we migrate to avoid as many issues as possible.
While I appreciate you identifying areas of improvements as you work on this task, the right way to handle this is by cutting issues to track them.
| // TODO(forrest) [correctness]: need to allow aliases to be provided over CLI | ||
| alias := "TODO" |
There was a problem hiding this comment.
Need to address, not sure if the alias should be passed by the client or picked by the CLI.
There was a problem hiding this comment.
If I remember correctly, alias is the replacement for name. How did we set names before?
There was a problem hiding this comment.
This was ported from the model package. We will be deleting the model package at which point the duplicated code will be remove.
| if opts.RunTimeSettings.DryRun { | ||
| // Converting job to yaml | ||
| var yamlBytes []byte | ||
| yamlBytes, err = yaml.Marshal(j) | ||
| out, err := helpers.JobToYaml(job) | ||
| if err != nil { | ||
| return fmt.Errorf("converting job to yaml: %w", err) | ||
| return err | ||
| } | ||
| cmd.Print(string(yamlBytes)) | ||
| cmd.Print(out) | ||
| return nil | ||
| } | ||
|
|
||
| executingJob, err := util.ExecuteJob(ctx, j, opts.RunTimeSettings) | ||
| api := util.GetAPIClientV2(cmd) |
There was a problem hiding this comment.
can we do things in a way where docker run and wasm run only generate job spec from flags, and eventually call job run or the same method that job run calls? I assume --dry-run, --follow, --id-only and similar flags apply to all and can be handled by job run
There was a problem hiding this comment.
Yes I can unify the method calls across the docker, wasm, exec, and run commands in this PR. Great way to de-dupe some of this code!
| engineSpec, err := models.DockerSpecBuilder(image). | ||
| WithParameters(parameters...). | ||
| WithWorkingDirectory(opts.WorkingDirectory). | ||
| WithEntrypoint(opts.Entrypoint...). | ||
| WithEnvironmentVariables(opts.EnvironmentVariables...). | ||
| Build() |
There was a problem hiding this comment.
we started to define the specs related to each publisher, source and executor under their package instead of models. This helps with initializing new types, validation and serde from and to models.SpecConfig. There is already a defined type under pkg/executor/docker/models/types.go:22 that should be used
There is also a builder in that package, but I don't think it is correct/useful as it is building a models.SpecConfig directly instead of EngineSpec
Also feel free to move EngineSpec from executor/docker/models to executor/docker
| // Function for validating the workdir of a docker command. | ||
| func validateWorkingDir(jobWorkingDir string) error { | ||
| if jobWorkingDir != "" { |
There was a problem hiding this comment.
We can add these validations under EngineSpec.Validate() method
| // users are not permitted to set, like ID, Version, ModifyTime, State, etc. | ||
| // The solution here is to have a "JobSubmission" type that is different from the actual job spec. | ||
| func JobToYaml(job *models.Job) (string, error) { |
There was a problem hiding this comment.
I hear your point. My preference is to delay any decision to change our API specs and have different ways to submit jobs until we collect more feedback about current APIs, and have more dedicated time to explore alternatives and align on the right option. Options might include splitting the job type into spec and state top level fields, where state hold the system defined fields, and spec is what the user submits. Again, I truly don't recommend figuring this out just yet.
A low hanging fruit is to introduce SanitizeForSubmission() method under models.Job that will clear out all fields that are reserved by the system so that something like bacalhau job describe <job_id> --output yaml | bacalhau job run would work
| // TODO(forrest) [correctness]: need to allow aliases to be provided over CLI | ||
| alias := "TODO" |
There was a problem hiding this comment.
If I remember correctly, alias is the replacement for name. How did we set names before?
| func DecodeSpecConfig[T any](spec *SpecConfig) (*T, error) { | ||
| params, err := json.Marshal(spec.Params) |
There was a problem hiding this comment.
how useful is this method? today we have each type implements its own way to decode SpecConfig to the right type. e.g. pkg/s3/types.go:92
| //nolint:gocyclo | ||
| func StorageStringToSpecConfig(sourceURI, destinationPath, alias string, options map[string]string) (*InputSource, error) { | ||
| sourceURI = strings.Trim(sourceURI, " '\"") | ||
| destinationPath = strings.Trim(destinationPath, " '\"") |
There was a problem hiding this comment.
same comment as to the publisher parser
| // WasmEngineSpec contains necessary parameters to execute a wasm job. | ||
| type WasmEngineSpec struct { | ||
| // EntryModule is a Spec containing the WASM code to start running. |
There was a problem hiding this comment.
similar comment as in the docker engine spec
| // WasmEngineSpec contains necessary parameters to execute a wasm job. | ||
| type WasmEngineSpec struct { | ||
| // EntryModule is a Spec containing the WASM code to start running. |
There was a problem hiding this comment.
similar comment as in the docker engine spec
|
|
||
| // TaskNamePrefix is the prefix of a system generated task name. | ||
| TaskNamePrefix = "t-name-" | ||
|
|
||
| // JobNamePrefix is the prefix of a system generated job name. | ||
| JobNamePrefix = "j-name-" |
There was a problem hiding this comment.
I don't like this? Names are not system generated and our expectation is for the user to provide them as a reference to their jobs
There was a problem hiding this comment.
Yeah, me either, I just needed something to generate a name if one wasn't provided. What is the expected behavior if the user doesn't provide a name? The bacalhau job run command will fail, should docker run and wasm run do the same? Note, this would be a change in their existing behavior.
There was a problem hiding this comment.
You are right about job name. This PR should help #4005
For task, use main
frrist
force-pushed
the
frrist/cli/deprecate-model
branch
from
793da06
to
c796ccd
Compare
|
Scope of this change is larger than expected and there are some breaking changes in the new job spec. Descoping this to a smaller change via #4020. |
This change removes the
pkg/modeldependency from thedocker runandwasm runcommands and migrates both of them to the new API methods. The flags on each command remain unchanged.makes progress towards #3831 and #3832
TODO: