-
Notifications
You must be signed in to change notification settings - Fork 208
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
fix running in bash environment on windows #6715
base: main
Are you sure you want to change the base?
Conversation
Thanks @lainme for the PR. I am not familiar with windows, @edan-bainglass could you give this a look. I guess the cmdline is not the only problem that we cannot support aiida natively on windows? |
I have sucessfully run a simple example from aiida tutorial on windows. Here is what I do,
Here is some screenshot To run aiida in jupyter on windows platform
Here is the screenshot of running in jupyter notebook |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6715 +/- ##
==========================================
- Coverage 78.13% 78.12% -0.00%
==========================================
Files 564 564
Lines 42526 42549 +23
==========================================
+ Hits 33222 33236 +14
- Misses 9304 9313 +9 ☔ View full report in Codecov by Sentry. |
The previous modification only fixes the running of verdi commands and internal python code. New commits fix the running of simple external code (such as aiida-diff) under windows, and more verdi commands can run under powershell as well. To make external code works, one should also make some modification to git bash (or other bash system on windows):
In git bash, to use ps from procps-ng:
|
…to fix-windows-running
for more information, see https://pre-commit.ci
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.
Could you embed the suggested change in the pyproject.toml
? Then we can run the CI
pyproject.toml
Outdated
@@ -48,7 +48,8 @@ dependencies = [ | |||
'tqdm~=4.45', | |||
'typing-extensions~=4.0;python_version<"3.10"', | |||
'upf_to_json~=0.9.2', | |||
'wrapt~=1.11' | |||
'wrapt~=1.11', | |||
'chardet' |
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.
We just pinned most recent version
'chardet' | |
'chardet~=5.2.0 ; platform_system == "Windows"' |
and then you need to run uv lock --upgrade-package chardet
to put this into the uv.lock file.
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.
When you do this we can run the CI
I the current state of this PR on the CI on windows and more than a third of the tests now pass, before this PR nothing would pass. See test suite run https://github.com/aiidateam/aiida-core/actions/runs/12950900268/job/36124705490?pr=6729 The test suite seems to crash (at least the errors are not properly reported after the run finishes), because of Exception ignored in atexit callback: <bound method InteractiveShell.atexit_operations of <IPython.terminal.interactiveshell.TerminalInteractiveShell object at 0x0000019912B26CD0>>
Traceback (most recent call last):
File "D:\a\aiida-core\aiida-core\.venv\Lib\site-packages\IPython\core\interactiveshell.py", line 3957, in atexit_operations
tfile.unlink()
File "C:\hostedtoolcache\windows\Python\3.11.9\x64\Lib\pathlib.py", line 1147, in unlink
os.unlink(self)
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpasaqzo87test_hist.sqlite' Will need more time to actually mark the tests that are passing so we only run these ones in the CI. In any case I would not block this PR till we have something in the CI running windows. |
for more information, see https://pre-commit.ci
…to fix-windows-running
for more information, see https://pre-commit.ci
I also makes the daemonization in background works with a patched version of circus-0.18.0, but I don't have time to make the patch work with the master branch of circus. The basic idea is parent->call vbs->call bat->call child, then the child can run in background continuously. It's possible to implement this in aiida instead of circus, but the circus patch also fixed other stuff such as signal handling, etc. Here is the patched circus-0.18.0: https://github.com/lainme/circus/tree/0.18.0-fix-windows |
Thanks, I will have a look at your fork and see if we can monkey patch it. We also contribute to circus, so I can have a look to merge it there. Regarding this PR I think we just need to add chardet to the conda environment.yml. I'll try to push something |
Conda does not seem to have for the moment a way to make dependencies OS specific. There is the selector mechanic which some tools related to conda support like conda-lock which uses the same syntax conda uses to define metapackages. So I will just go with this to mark the package so one can in principle extract it out in the future. For reference, in conda there is an ongoing discussion about supporting selectors suhttps://github.com/conda/conda/issues/8089 |
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.
Will just have final look on Monday and then merge
With this fix, aiida verdi commands can run on bash environment on windows, such as git bash