Conversation
|
Thanks for the ping guys. I'm focussing entirely on |
|
@danielhollas at this stage, do you see any issue with just using |
|
@mbercx no particular reason other than the fact that this is largely untested and we could run into unforeseen issues (in which case having a plain pip as an escape hatch would have helped). But I am happy to simplify this if you think it's a better path forward (and any issues can be fixed in subsequent PRs). LMK what you think. |
Hmmm, would the simplifications to the code really be worth dropping the most standard tool for this? I'm a bit hesitant about this... I know for the user outside, it would look basically the same: make |
|
@GeigerJ2 I don't have a strong opinion about this but note that we install uv into the venv, so the user should not even notice there is a difference, besides the speed. One thing I did not consider in this PR, of we fully switch to uv, we might need some extra code to handle existing projects. |
aiida_project/project/venv.py
Outdated
| exist_ok=True, | ||
| parents=True, | ||
| ) | ||
| venv_command = [self.uv_path, "venv", "-p", f"{python_path.resolve()}", str(self.venv_path)] |
There was a problem hiding this comment.
Maybe we can just create the environment with the regular venv module? This doesn't take a lot of time, right?
There was a problem hiding this comment.
Well, in takes a whole second on my machine (kind of surprising), which is actually a lot in terms of uv speed. I don't think there's a good reason NOT to use uv here tbh.
|
Warning The message below is off-the-cuff, and may contain mistakes. Also I just found out that you can use these admonitions on GitHub and I love it. 😍
Frankly, I think so, yes. Working on AiiDA has made me very wary of:
I would:
Note: I know we can also install other packages with the
If we do the changes I propose above, would we? The only thing we'd change is installing |
|
Note I totally agree with your excitement about the admonitions. 🤣
Indeed, that's already done in this PR.
I mean, uv should work perfectly fine for installing whatever package so I don't think we need to remove this feature, at least it's not pre-requisite to merging this PR.
Yeah, that really should not happen, as we're using
Right. I am not that familiar with this project --- but indeed if the package installation only happens at project creation then we should be fine. We just need to be mindful of the fact that the old venvs will not have uv installed in them --- I guess this is fine at this point, but if we ever added a feature that would be touch existing venv we would have to think about that. I agree with the general sentiment of keeping things simple. When I cooked up this PR during the last year coding week, I wasn't even sure if this feature would be wanted, which is why I went with making it optional. But I agree that the risk here should be small if we test things enough. I'll work on simplifying this PR this week. |
Completely agree! I also wouldn't expect issues, to be honest. But #30 describes several other reasons why I think we should remove that feature.
If you have time, great! I'm also back to coding quite a bit more lately, so happy to help if needed. |
0d6e8cb to
6f9e231
Compare
Just note that they don't appear properly upon "quote reply" :D
There goes my excitement of learning about the Strategy pattern, but, fair enough, we all know this pain all too well.
OK, if we can guarantee that PS: Sorry for closing the PR, ended up clicking that button with my touchpad on page reload... |
Don't you dare rain on my parade! 😤
Haha, I'm all too familiar with the trap of wanting to over-engineer something. So I won't cast the first stone. |
57e3fc9 to
af4f927
Compare
|
Alright, this is ready for a next round of review. The PR is now delightfully simple. 👼 One open question: The If it is not, we should stop early with a notification message (that seems to be the most reasonable to me). This is important for this PR because |
I wouldn't say it's intended. This package was mostly my personal tool at first, so safety and UX wasn't too much of a concern. That said, I have used this in the past to overwrite the virtual environment while keeping the project directory (and the As usual: I'll write my OCD commit messages as I review and understand the changes, ok if I reorganise? :) |
Very sexy indeed! But errmmm, wasn't it getting late there? I feel I have to stop reviewing your PRs to help improve your sleeping pattern. 😅 |
|
Just a note here that, if I |
Okay, for now I've added code that simply returns early if a project exists already. Users can always nuke their venv manually if needed. Caution The main scary thing here is that if you happen to provide a wrong path |
First of all I would like to reiterate how much I love these admonitions. 😍 But yeah, fair point. You'd have to pass |
mbercx
left a comment
There was a problem hiding this comment.
Sweet, great work @danielhollas! Testing this locally has been an absolute bliss with the improved speed, can't wait to get this released. 🚀
Just left a few nitpicks, but then this is good to go for me! I'd split up the commits as usual, and then have you double check the messages, if that's ok?
| if aiida-project create testproject; then echo "ERROR: Attempting to overwrite an existing project should fail!"; fi | ||
| if aiida-project create ""; then echo "ERROR: Attempting to create a project with empty name should fail!"; fi | ||
|
|
||
| # NOTE: The cda bash function does not seem to work in GitHub runners so we execute it manually |
There was a problem hiding this comment.
I didn't catch this before, but have you tried source-ing the $HOME/.bashrc? The aiida-project init command will add the cda command there, but the user always has to source or open a new terminal.
There was a problem hiding this comment.
Yes, I did try and couldn't make it to work.
Yep, please feel free to finish this, no need for me to double check, I will not have time to review this evening and will be glad to have this from my plate.
Yes, it's very nice. 🎉 |
I understand, you've been pumping quite a bit of time into |
8cadd47 to
f0dbed9
Compare
Switch to using `uv` to set up the virtual environment and install packages for `VenvProject`. `uv` is added as a dependency, and the executable installed in the Python environment where `aiida-project` is installed is used. In rare cases where this does not work (e.g. installations in the global environment), we try to fall back on looking for `uv` in the `PATH` via `which` and exit with an error in case that fails. Although `uv` is still relatively new, it's currently used in most of our CI's, and hence in case there is an issue we should discover it quite quickly. In case there is still a need to have `pip` as a fallback, we can add a configuration option that allows the user to do so. Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
Currently the user can pass an already existing project or an empty string as the project name of the `create` command. This can lead the user to potentially lose all their `aiida-project` environments. Here we check for both cases and exit with an error if true. Moreover, we also catch any error raised by the `BaseProject.create()` method and echo the details to the user, instead of just silently failing. Co-authored-by: Daniel Hollas <daniel.hollas@bristol.ac.uk>
mbercx
left a comment
There was a problem hiding this comment.
Thanks again @danielhollas! Time for a release, I'd say. :) 🚀


tl;dr with
uv(and warm uv cache), creating a new aiida project takes around 3 seconds (unlike 30-40 seconds with pip). You can finally have a tagline "Create AiiDA project in a jiffy!"After more testing I think it would be reasonable for this to be a new default, but would keep pip as a fallback option (we've been usinguvin CI for a while and haven't had much issues).Closes #27