-
Notifications
You must be signed in to change notification settings - Fork 401
feat(task): --default_environment flag for specifying the default environment for a task
#5164
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
Conversation
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
|
@ruben-arts This is ready to be reviewed. |
schema/model.py
Outdated
| default_environment: NonEmptyStr | None = Field( | ||
| None, | ||
| description="A default environment to run the task", | ||
| json_schema_extra={ |
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.
actually, with the above comment about EnvironmentName, this json_schema_extra could be removed entirely: the lack of title is generally fine (unless really explaining some obtuse term), and the regex would flow more predictably from the typing.
json_schema_extra should probably be reserved for really tricky things that don't have an "elegant" pydantic analogue.
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.
The only reason I added json-scheme-extra was so that I could specify the title as default-environment, not Default-Environment, in schema.json (which seems to be the default generation); however, you are right, the title hardly matters here. Thanks for the review.
Signed-off-by: Pradyot Ranjan <[email protected]>
| description: EnvironmentName | None = Field( | ||
| None, | ||
| description="A short description of the task", | ||
| examples=["Build the project"], |
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.
Also, remove the example field, as it may be misleading/not required.
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.
@bollwyvl Removed the json-scheme-extra part.
Signed-off-by: Pradyot Ranjan <[email protected]>
|
I updated the branch to fix some other issues, for testing. |
|
Hey @prady0t, I was testing with this toml file: [workspace]
channels = []
platforms = ["osx-arm64"]
[tasks]
test = "echo test"
test2 = "echo test2"
dep.depends-on = ["test3", "test6"]
[feature.test.tasks]
test3 = { cmd = "echo test3", default-environment = "three" }
test4 = "echo test4"
[feature.test2.tasks]
test5 = "echo test5"
test6 = { cmd = "echo test6", default-environment = "four" }
[environments]
one = []
two = ["test"]
three = ["test2", "test"]
four = ["test2"]
five = ["test", "test2"]
six = { features = ["test"], no-default-feature = true}
seven = { features = ["test2"], no-default-feature = true}But in no case would it actually use the default-environment. Could you test it yourself and add some tests. The other decisions you made look fine to me, so I'm looking forward to testing them. |
|
For me, it just gives different environments where tests could be run. Let me find a way to use the specified default environment when the task can be solved for multiple environments. |
I might have misunderstood, but isn't that the whole idea of this feature? |
Exactly. Currently, the default-environment is applied after task resolution, and hence, if multiple environments are specified, it prompts the user. Due to task ambiguity There's another bug for the same manifest; remove the default-environment part for test3 and run with the The current bug can be fixed by adding a check for a task with default_environment in |
Signed-off-by: Pradyot Ranjan <[email protected]>
…into add-default-channel
| // If any of the candidate tasks declares a `default-environment` that | ||
| // corresponds to one of the candidate environments, prefer that | ||
| // environment automatically. | ||
| if let Some(idx) = problem.environments.iter().position(|(env, task)| { | ||
| if let Some(default_env_name) = task.default_environment() { | ||
| default_env_name == env.name() | ||
| } else { | ||
| false | ||
| } | ||
| }) { | ||
| return Some(problem.environments[idx].clone()); | ||
| } | ||
|
|
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 works for the currently discussed bug, but now you cannot override this using -e flag.
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 the priority should be
--environmentin thepixi runcli (like it's now)- the
default-environmentfield in thetaskdefinition. (new) - the
defaultenvironment when possible (like it's now) - the disambiguation function (like it's now)
Signed-off-by: Pradyot Ranjan <[email protected]>
|
@ruben-arts these changes should be good to go. I've also added a few tests. |
crates/pixi_task/src/task_graph.rs
Outdated
| // If the task itself declares a `default_environment`, prefer that | ||
| // environment when constructing the root node (if it exists in the | ||
| // project). Do not fail if the named environment is missing; fall | ||
| // back to the resolved `run_env`. | ||
| if let Some(default_env_name) = task.default_environment() | ||
| && let Some(env) = project.environment(default_env_name) | ||
| { | ||
| run_env = env; | ||
| } | ||
|
|
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.
Completely removed these checks since the disambiguator already checks for it, this helps explicitly declared environment. to take over
| } else { | ||
| task_env | ||
| }; | ||
|
|
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.
Same reason as above
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
|
The latest changes don't work for overriding the default environment yet. [workspace]
channels = []
platforms = ["osx-arm64"]
[tasks]
test = { cmd = "echo test", default-environment = "test" }
[feature.test.dependencies]
# pytest = "*"
[environments]
test = ["test"]Gives: While I expected it to run in |
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
Signed-off-by: Pradyot Ranjan <[email protected]>
|
Added a check for when the task is in default and default-environmnet is added. |
--default_environment flag for specifying the default environment for a task.--default_environment flag for specifying the default environment for a task
|
tested with both |
nichmor
left a comment
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.
removed a small empty field from tests - I like it!
Description
With this PR, a user can specify a default environment per task. Some work is still left in:
--default_environmentflag with pixi task add, should also add the default environment to pixi.tomlFixes #5140
How Has This Been Tested?
Using a pixi.toml file with the option
default_environmentruns the task in that environment (if it can).AI Disclosure
Some detailed comments are generated by Github copilot for a more accurate description.
Checklist:
schema/model.py.