ewoks install: fresh env by default with the pip-venv package manager - #332
ewoks install: fresh env by default with the pip-venv package manager#332woutdenolf wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
payno
left a comment
There was a problem hiding this comment.
Thanks for the detailed documentation.
To be honest, it's hard for to be very critical about the PR. It would require a deeper knowledge of this system and/or package managers. And this would require quiet some time for me. I think Loïc will give a better review, but feel free to merge if needed.
My main concern is trying to reduce long-term maintenance. I'm very worried about the effort needed to handle all the use cases. IMO, forcing "reproducers" to use the same package manager isn't a big requirement, compared to the time/energy we'd need to spend making sure we don't end up with corner cases — and I'd advocate for this, at least.
| """Location of the environment with this name, inside a root directory of | ||
| the user or inside the root directory of this package manager. | ||
| """ | ||
| return Path(root or self.environments_root()).expanduser() / name |
There was a problem hiding this comment.
Hard to be very accurate at the moment because I don't know this part of the code yet.
But having this or in Path(root or self.environments_root()).expanduser() / name sounds a bit weird.
Makes me think that the environment location might not be static. From what I get from claude he/she says this will happen when user override the default location. But in this case I would expect environments_root to also be redefined.
There was a problem hiding this comment.
This function takes the root (optional coming from the user) or the environment root, expands ~ and return the child name. Not sure what the problem is.
|
|
||
| @abstractmethod | ||
| def create_environment( | ||
| self, location: Path, python_version: Optional[str] = None |
There was a problem hiding this comment.
in environment_location we use the term name (and scalar). I guess you want to open the API to handle any location. But this might make thinks complex to maintain.
There was a problem hiding this comment.
I don't get this comment. The name of an environment and the location (directory provided by the user: an environment prefix or a project) are two very different things.
| managers = _managers() | ||
| scores = {name: (manager.PRIORITY,) for name, manager in managers.items()} | ||
| manager = _first_available(managers, scores) | ||
| if manager is None: | ||
| raise RuntimeError("No known package manager installed or available") | ||
|
|
||
| return manager |
There was a problem hiding this comment.
Not simple but should we consider this case ? the original manager not being here ?
To be honest I am a bit afraid that trying to bridge the gap between the different package manager is a pain we might want to avoid (at least to start. Because once we engage ourself this way we must continue).
There was a problem hiding this comment.
Requirements are always stored in two ways:
- python's distribution info extracted from
importlib.metadata - package manager specific (requirements.txt for pip for example)
We already need to fallback to importlib.metadata, especially for pip, since the result of pip freeze is not always installable (e.g. points to local paths).
Since we already need this fallback mechanism even when the producer and installer both use the same package manager, cross-manager support comes for free (using the importlib.metadata fallback).
| Install any package that provides :term:`tasks <Task>` instead of | ||
| ``ewoks`` for a real :term:`workflow`. | ||
|
|
||
| Re-producer side |
There was a problem hiding this comment.
I think there's a corner case worth highlighting.
Right now, if I'm not mistaken, nothing prevents a user from converting a workflow and ending up with a Python environment that fully supports running it — which is very convenient.
However, if that user then shares it with a third party, the installation becomes useless.
There was a problem hiding this comment.
However, if that user then shares it with a third party, the installation becomes useless.
Don't get it. Shares what with a third party?
- user1 generates a JSON file (ewoks convert) and share it with user2.
- user2 receives an JSON file and installs (ewoks install) and executes (ewoks execute).
We are not sharing the environment, we are sharing the workflow (JSON or whatever).
Co-authored-by: payno <payno@users.noreply.github.com>
In #65 we changed the graph attributes
to
The final goal is to support more package managers than pip (uv, poetry, pixi and conda).
This PR is another step:
See docs on how this PR affects
ewoks installandewoks convertdoc/howtoguides/requirements.rstdoc/tutorials/install.rstdoc/tutorials/install/pip_venv.rstThis was cherry-picked from #330 to make the review easier. Still a large PR, sorry. The other PRs already adds uv, pixi, conda and poetry. This PR already lays the groundwork for those in terms of core, test and doc infrastructure.
FYI
python -m ewoks._requirements.__init__does a pip gathering in your current environment. Might be useful for the reviewing.