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

[feat] Add Windows support #2415

Open
wants to merge 11 commits into
base: main-bckp
Choose a base branch
from
Open

[feat] Add Windows support #2415

wants to merge 11 commits into from

Conversation

2-5
Copy link

@2-5 2-5 commented Dec 8, 2022

Added support for Windows build.

With the associated aimrocks PR, all the Cython extensions are now building and I can import the aim package on Windows.

(venv) X:\Clone\aim-windows\aim>aim version
Aim v3.15.1

I'm now trying to get the tests running.

I will further commit to this PR so please hold on merging it.

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2022

CLA assistant check
All committers have signed the CLA.

@2-5
Copy link
Author

2-5 commented Dec 8, 2022

I managed to get everything up and running - created an aim repo, did a test run, use the web-ui to see it:

aim_1

aim_2

aim_3

One issue I encountered: after about 2 minutes of aim up running these errors start appearing in the terminal:

aim_4

@mihran113
Copy link
Contributor

Hey @2-5! Huge thanks for the effort. Could you please push the latest changes as well? I'll build it on my local windows machine and try to investigate the issue. As it's not clear from the logs what's causing the indexing issues.
Also we have a lot of hard-coded '/'s in the path operations, will check those as well :)

@2-5
Copy link
Author

2-5 commented Dec 8, 2022

Everything is pushed.

But for now you need to manually mess around with the .whl files - build both aim and aimrocks with these PR steps, but then manually take the built .whl files and install them in a new venv:

pip aim-3.15.1-cp310-cp310-win_amd64.whl
pip aimrocks-0.2.1-cp310-cp310-win_amd64.whl

Since aimrocks for Windows is not on PyPI yet, I used this requirements.txt to install all the aim dependencies in the same venv. Basically copied everything from the Linux wheel metadata on PyPI (aim-3.15.1-cp310-cp310-manylinux_2_24_x86_64/aim-3.15.1.dist-info/METADATA:Requires-Dist)

aim-ui==3.15.1
aimrecords==0.0.7
# aimrocks==0.2.1
cachetools>=4.0.0
click>=7.0
cryptography>=3.0
filelock>=3.3.0
numpy>=1.12.0
protobuf>=3.11.0
psutil>=5.6.7
py3nvml>=0.2.5
RestrictedPython>=5.1
tqdm>=4.20.0
aiofiles>=0.5.0
alembic>=1.4.0
fastapi<0.68.0,>=0.65.0
jinja2>=2.10.0
pytz>=2019.1
SQLAlchemy>=1.4.1
uvicorn>=0.12.0
Pillow>=8.0.0
protobuf<4.0.0,>=3.9.2
packaging>=15.0
python-dateutil
requests
grpcio>=1.42.0
async-exit-stack>=1.0.0; python_version < "3.7"
async-generator>=1.0; python_version < "3.7"

Also we have a lot of hard-coded '/'s in the path operations, will check those as well :)

On the Python side / works just fine on Windows. Not sure about rocksdb. One issue might be if you directly compare paths with / or \ in them, something along this line: '.aim/aim_db' == os.path.join('.aim', 'aim_db')

I see some logger.info in the code where it fails, I tried running aim -v up to enable verbose mode, but it doesn't seem to enable INFO level for logger.

@mihran113
Copy link
Contributor

Thanks a lot, I'll have a look. What about logs, it's enabled by adding a --log-level=info argument to the command.

@2-5
Copy link
Author

2-5 commented Dec 8, 2022

Thanks, I've found it too, but it doesn't seem to work. It looks that there are multiple logging levels and formatters in play (uncolored INFO, green INFO and no WARN level tag):

aim_5

@2-5
Copy link
Author

2-5 commented Dec 8, 2022

I have discovered the issue, aim.storage.locking uses a filelock.UnixFileLock, it needs to use filelock.WindowsFileLock.

aim_6

NotImplementedError has an empty str representation, which is why the error was not visible in the log:

>>> e = NotImplementedError()
>>> str(e)
''

https://github.com/aimhubio/aim/blob/main/aim/storage/locking.py#L158

@2-5
Copy link
Author

2-5 commented Dec 8, 2022

The aim test suite is now passing. Two tests are being skipped. One of them was because I didn't have dvc installed. I installed it, and it fails, but it doesn't look to be Windows related:

FAILED
tests/integrations/test_dvc_metadata.py::TestDVCIntegration::test_dvc_files_as_run_param -
AttributeError: <module 'aim.sdk.objects.plugins.dvc_metadata' from
'H:\\Clone\\aim\\test\\.pyenv\\lib\\site-packages\\aim\\sdk\\objects\\plugins\\dvc_metadata.py'>
does not have the attribute 'Repo'

One remaining issue with the tests is that cleanup fails - conftest.py:_cleanup_test_repo raises PermissionError when calling shutil.rmtree(path) because the Repo is still open and holds locks on the files (you can't delete locked files on Windows). I tried both calling repo.close() and Repo.rm(path), but it doesn't seem to work.

@2-5
Copy link
Author

2-5 commented Dec 9, 2022

I have added /aim/docs/source/using/windows.md with full instructions on how to install aim on Windows.

It assumes that these two PRs are merged in.

Until then, you can replace git clone https://github.com/aimhubio/aim.git and git clone https://github.com/aimhubio/aimrocks.git in the scripts with git clone -b windows-build https://github.com/2-5/aim.git and git clone -b windows-build https://github.com/2-5/aimrocks.git.

There are no further commits from me to be done, aim seems to work correctly on my computer, so you can now test these PRs and see if it's possible to merge them.

@gorarakelyan
Copy link
Contributor

@2-5 huge thanks! We will review it asap. Also, I guess, we should check if all the paths are passed via Pathlib and make sure no other platform specific inconsistencies exist.

@cmoski
Copy link

cmoski commented Jun 8, 2023

Any progress on this? Trying to run on top of a Windows stack and running into issues. Would be nice to be able to have this part of the mainline.

@Yarin-Shitrit
Copy link

Any progress on this? Trying to run on top of a Windows stack and running into issues. Would be nice to be able to have this part of the mainline.

+1

@flaviuvadan
Copy link
Contributor

@gorarakelyan any chance you can provide input here? I wonder if this could be part of the next patch. Would love to use it!

@flaviuvadan
Copy link
Contributor

@SGevorg or @gorarakelyan bump! Trying to get an estimate of when this might be released given the interest

@xquyvu
Copy link

xquyvu commented Sep 5, 2023

I'm looking forward to windows support too!

@srfiorini
Copy link

srfiorini commented Dec 7, 2023

Looking forward for windows support as well.

@EtienneT
Copy link

This project look so great, it would be really nice to have it working on windows too!

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.

None yet

10 participants