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

enh(dev): Make default environment the developer environment #6456

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

hoxbro
Copy link
Member

@hoxbro hoxbro commented Nov 21, 2024

This PR does two main things:

  1. Merge the features of test and example into optional as the overlap in dependencies is large.
  2. Make the default environment the recommended developer environment for developers.

@maximlt, I will update the docs with the dev recommendation if you agree with the approach.


Image:
001
002
003

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.50%. Comparing base (93040aa) to head (074033c).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6456   +/-   ##
=======================================
  Coverage   88.50%   88.50%           
=======================================
  Files         323      323           
  Lines       68631    68631           
=======================================
  Hits        60741    60741           
  Misses       7890     7890           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

I think that's a good approach. Maybe look for a better name instead of optional? Is the CI going to use the dev environment or it's just for contributors?

@hoxbro
Copy link
Member Author

hoxbro commented Nov 21, 2024

Maybe look for a better name instead of optional?

Open for suggestion only reason I took it was it looked nice in the toml file:
image

Is the CI going to use the dev environment or it's just for contributors?

As is, it would not be run in CI. Have updated the title with enh(dev) to reflect this better.

@hoxbro hoxbro changed the title ci: Add dev environment enh(dev): Add dev environment Nov 21, 2024
@maximlt
Copy link
Member

maximlt commented Nov 21, 2024

Was it required to gather the test and example dependencies in one feature? Genuine question, I don't know much about pixi yet. I see optional is part of the docs environment so it sounds possible it'll install dependencies not required to build the docs.

@hoxbro
Copy link
Member Author

hoxbro commented Nov 21, 2024

Was it required to gather the test and example dependencies in one feature? [...] I see optional is part of the docs environment so it sounds possible it'll install dependencies not required to build the docs.

No. I think the overlap is significant enough that there is no need to have them in two different features. Other than the docs environment, every other group had both of them in it. And for the docs environment, the extra dependencies are dask-expr, nbconvert, spatialpandas, and tsdownsample. The others either are implicit dependencies or are also required by the docs feature.


Maybe we should add an extra feature called development, which has dev-only features/commands in it. At the top of my head, I would add Jupyter Lab and maybe a task called pixi run lab (or similar).

@maximlt
Copy link
Member

maximlt commented Nov 22, 2024

No. I think the overlap is significant enough that there is no need to have them in two different features. Other than the docs environment, every other group had both of them in it. And for the docs environment, the extra dependencies are dask-expr, nbconvert, spatialpandas, and tsdownsample. The others either are implicit dependencies or are also required by the docs feature.

I don't understand the need to merge the tests and examples environment? It'd make sense to me if they would 100% overlap but it's not the case. They quite overlap today but this might change. You're also going to loose flexibility. I'm also concerned with this practice spreading to all the repos.

Maybe we should add an extra feature called development, which has dev-only features/commands in it. At the top of my head, I would add Jupyter Lab and maybe a task called pixi run lab (or similar).

Ah so ok now we just have a new dev environment, so people are supposed to run pixi run test-unit -e dev?

@hoxbro hoxbro marked this pull request as ready for review November 25, 2024 16:00
@hoxbro hoxbro requested a review from maximlt November 25, 2024 16:00
Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

I think the docs should still be improved. It'd be a good principle to imagine that people won't really read the text between the commands, and won't even read the whole guide before simply starting copy/pasting commands and running them (that's why I do...).

The first command is pixi install which creates a default environment that doesn't seem needed (when created, VSCode immediately suggests me to select it as the default interpreter for the project). Then you've got pixi run download-data, pixi run sync-git-tags and pixi run install. The last one will make an editable install in the default environment, which again doesn't look very useful. Finally we get to the real deal :) with pixi run -e dev install!

Next we have pixi run lint that offers me two environment options lint and dev. Of course I don't think much and just select the first one, which is going to create another not very useful lint environment. Followed by pixi run test-unit which offers 6 options, dev being the last one. There's practically no chance I select it, that's way too much clicking :) So I'll end up with yet another environment.
image
And so on ... :)

My main feedback would be to add -e dev almost everywhere! Starting with pixi install -e dev, and pixi run -e dev install, etc. Most contributors shouldn't have to think about all these environments (and don't need to use 15GB of disk space for them). They only need to run a subset of the commands we run on the CI. The guide should then end with a section for advanced users, far from the eyes of a normal contributor, saying "By the way if you want to run the tests in another environment (e.g. diff Python version), execute pixi run test-unit and pick the environment of your choice".

@hoxbro hoxbro changed the title enh(dev): Add dev environment enh(dev): Make default environment the developer environment Nov 26, 2024
.github/workflows/test.yaml Outdated Show resolved Hide resolved
@hoxbro
Copy link
Member Author

hoxbro commented Nov 26, 2024

I have fixed your main observation from the last comment, getting to the real deal for developers. The default environment will be the "dev" environment, meaning there will be no selection menu and no adding -e dev to each command. This comes at the cost of a little more complicated pixi.toml file.

I have made a dev doc build.

Copy link
Member

@maximlt maximlt left a comment

Choose a reason for hiding this comment

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

Sorry for mixing things up but while we're at it... should we ask people to run pixi run lint? Seems more like something you'd do in case you don't manage to install pre-commit. Maybe put it in an admonition?

doc/developer_guide/index.md Outdated Show resolved Hide resolved
Comment on lines +110 to +112
The `default` environment is meant to provide all the tools needed to develop HoloViews.

```bash
pixi run install
```
This environment can be created by running `pixi run install`, which will set up the environment and make an editable install of HoloViews.
Copy link
Member

Choose a reason for hiding this comment

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

The docs already mention pixi run install, that feels a little redundant. I'd just remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is ok to repeat ourselves, especially when it is essential to getting started.

doc/developer_guide/index.md Outdated Show resolved Hide resolved
doc/developer_guide/index.md Outdated Show resolved Hide resolved

### Jupyter Lab

You can launch Jupyter lab with the `default` environment with `pixi run lab`.
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should let contributors know that they should run this command when:

  • they need to edit the docs (as building the docs is usually overkill)
  • they need to debug example tests

Either here, or perhaps preferably in the relevant sections.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added some extra text.

@hoxbro
Copy link
Member Author

hoxbro commented Nov 26, 2024

should we ask people to run pixi run lint? Seems more like something you'd do in case you don't manage to install pre-commit. Maybe put it in an admonition?

I have kept it but added an admonition that you can use pre-commit directly.

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.

2 participants