Skip to content
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

misc: use default env if task available in it. #772

Merged

Conversation

ruben-arts
Copy link
Contributor

This addresses @pavelzw issue in #767

Which means that if a task is in the default environment we always automatically run it from that environment.

@baszalmstra
Copy link
Contributor

Mm im not sure about this implementation.

Pavels usecase makes sense but this impl will also select the default env if two tasks with the same name are defined which I think is not correct.

@ruben-arts
Copy link
Contributor Author

So your saying that pixi run test should still spawn the disambiguate check? With this toml:

[tasks]
test = "cargo test"

[feature.test.tasks]
test = "cargo test --all-features"

[environments]
test = ["test"]

@baszalmstra
Copy link
Contributor

Exactly!

@ruben-arts
Copy link
Contributor Author

This should change it, @pavelzw could you check if this still fits your idea?

.parsed
.features
.iter()
.filter(|(name, _feature)| name.as_str() != "default")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add a function to check if a feature is the default instead of comparing strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I think I got distracted

src/project/manifest/environment.rs Show resolved Hide resolved
@pavelzw
Copy link
Contributor

pavelzw commented Feb 4, 2024

could you check if this still fits your idea

unfortunately not, running the code with this pr still prompts me for the environment even though the task test is the same in all envs and test is present in the default environment.

[project]
name = "test-project"
channels = ["conda-forge"]
platforms = ["linux-64", "osx-arm64", "osx-64", "win-64"]

[dependencies]
python = ">=3.11"

[tasks]
postinstall = "echo running postinstall"

[feature.py311.dependencies]
python = "3.11.*"
[feature.py312.dependencies]
python = "3.12.*"

[feature.test.dependencies]
pytest = "*"
[feature.test.tasks]
test = "echo testing now"

[environments]
default = ["py312", "test"]
py311 = ["py311", "test"]
py312 = ["py312", "test"]

This is a minimal example of what i'm trying to achieve.
pixi run postinstall runs in the default env but pixi run test still prompts for the environment even though it shouldn't imo. (tested on 121995b (#772))
Maybe include this minimal example in the tests once the issue is resolved?

@ruben-arts
Copy link
Contributor Author

My initial commit did exactly that but @baszalmstra`s comment told me I needed to do it differently, I can see 2 use cases.

  1. When you run on a cuda machine you want to get the cuda env instead of the default. So then getting the question would help.

  2. In the polarify example you probably mainly use the envs in CI so locally you don't want the extra input moments.

In your example @pavelzw you could move the test task into the default feature. But that could indeed give weird results as a "lint" env (if that was separately defined) could then also include that task.

@pavelzw
Copy link
Contributor

pavelzw commented Feb 4, 2024

I think prompting only if the test task is defined multiple times, i.e., there are different test tasks could be a solution? WDYT @baszalmstra?

@ruben-arts
Copy link
Contributor Author

So @pavelzw you are saying:
Prompt when:

[tasks]
test = "pytest"

[feature.test.tasks]
test = "pytest -s"

[feature.prod.tasks]
run = "python start.py"

[environments]
default = ["test"]
test = ["test"]
prod = ["prod"]

But execute in default env when:

[tasks]

[feature.test.tasks]
test = "pytest -s"

[feature.prod.tasks]
run = "python start.py"

[environments]
default = ["test"]
test = ["test"]
prod = ["prod"]

@pavelzw
Copy link
Contributor

pavelzw commented Feb 4, 2024

Yes, that would at least solve my use case 😄

@baszalmstra
Copy link
Contributor

yeah that sounds perfect! 👌

@ruben-arts
Copy link
Contributor Author

As I'm trying to re-implement this I'm trying to write down what the rules are and they are so confusing even to me while developing the feature that I think it will never be intuitive to the user.

I would like to keep it to these rules:

  • Is the task in the default environment, run the default environment
  • Is the task not in the default environment, but only one environment, run that.
  • Is the task not in the default environment, and in multiple environments, spawn selection ui/error in non tty.

I think we should discuss an extra rule set that excepts some logic, like I described in #750.

@baszalmstra
Copy link
Contributor

I think amending with

  1. If the task is in de default evironment and there is not other task with the same name (in any environment), run the task in the default evironment.

  2. Otherwise ask the user

should solve our issue.

@ruben-arts
Copy link
Contributor Author

I assume that now work, check the tests.

Comment on lines 124 to 132
let is_unique_task = self.project.environments().iter().all(|env| {
match env.task(name, self.platform) {
Ok(task) => task == default_env_task,
Err(_) => true,
}
});

if is_unique_task {
return Ok((default_env, default_env_task));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense to turn this expression around: NOT any other task. Instead of all environments all refer to this task. (faster to find the negative)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not entirely happy that we have to compare Tasks now because this now still fails:

[tasks]
bla = "echo foo"

[feature.other.tasks]
bla = "echo foo"

[environments]
other = ["other"]

Which is arguably indeed still ambiguous. I think you should look at the unique features of an environment and see if it contains a task with the same name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm lost, I though that is what you described. Could you give an toml/shell example of If the task is in de default environment and there is not other task with the same name (in any environment), run the task in the default environment.

I think I'm parsing it wrong

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[feature.other.tasks]
bla = "echo foo"

[environments]
default = ["other"]
other = ["other"]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you want that to spawn the selector, that would go against pavels example right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I think its the same? Let me explain over video!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it now does what we all expect 😅

@baszalmstra baszalmstra merged commit 962b0cb into prefix-dev:main Feb 8, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants