Skip to content
This repository has been archived by the owner on Dec 5, 2024. It is now read-only.

Improve dependency management and testing. #7

Merged
merged 19 commits into from
Mar 14, 2022

Conversation

bryan-hoang
Copy link
Owner

Primarily adds pipenv to the project to manage its dependencies. Now, running make install-dev should be all that's needed to get the project's dependencies set up on 32-bit raspberry pis.

A notable change needed to make this work is adding a locally compiled wheel under a vendors folder to install PyTorch.

Closes #6.

Other changes:

  • Adds pytest as the test runner.
  • Adds a Makefile for common commands.
  • Update set up instructions in the README

Bryan Hoang added 8 commits February 8, 2022 17:08
To install `torch` in the planned testing/production environment, a
preocompiled wheel for the specific python version and architecture was
added. Also changed the python version required from 3.7 to 3.9.
With the introduction of `pipenv`, `requirements.txt` is no longer
needed.
To help make running tests easier.
@bryan-hoang bryan-hoang added the enhancement New feature or request label Feb 9, 2022
@bryan-hoang bryan-hoang self-assigned this Feb 9, 2022
Copy link
Collaborator

@eparker-wavefin eparker-wavefin left a comment

Choose a reason for hiding this comment

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

One additional thing, do we need to encrypt the torch wheel for security reasons? I can't think of any reason to atm but just wanted to make sure that's considered.

README.md Outdated Show resolved Hide resolved
src/client.py Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
pytest.ini Show resolved Hide resolved
Pipfile Outdated Show resolved Hide resolved
@jaredmcgrath
Copy link
Collaborator

On a note of a better way to torch for the ARM32 architecture:

  • Are there for sure no versions of torch and/or python that have PyPi-hosted wheels? This is the easiest option, if it's available
  • It seems like there's a lot of builds in the linked GitHub repo ljk53/pytorch-rpi. Would one of those work if we downgrade the python version to 3.8.x or 3.7.x? If so, that's easier than managing our own builds, and I would highly recommend going that route
  • Alternatively, I would suggest setting up releases on GitHub.
    • Include the source code to compile torch for the pi/ARM32 architecture (in this repo, or in a separate repo)
    • Include the relevant Makefile/other build tooling that can be ran on a pi to create the torch binary
    • Publish a new GitHub release for that repo, including the zipped/tarred source code + the compiled binary
      • This means we'll have a self-hosted URL on GitHub to point to for the install via pipenv
  • One more item worth mentioning: we could explore publishing a wheel to PyPi or some other platform that hosts packages. Not sure if this is difficult or feasible.

Once that is sorted out, we can manage the Pipenv using platform-specific dependencies. I've got about ~60 mins of Pipenv experience, so I am no expert. But some quick digging led me to a couple different resources.

Based on what I've read, we've got two solutions here:

  1. Multiple Pipfile + Pipfile.lock versions (one for ARM32, one for the rest at a minimum). This is unsophisticated but very easy to try. If complemented with a Makefile enhancement, it could be seamless
  2. Pour through the documentation and info above to learn advanced Pipenv features. Essentially, just need to specify different versions of the platform based on sys_platform and/or platform_architecture, etc. I spent an hour messing around with this, and I (personally) do not wanna explore this route anymore.

That's about all the info I've been able to find. Good luck!

Co-authored-by: Edan Parker <[email protected]>
@bryan-hoang
Copy link
Owner Author

@jaredmcgrath Regarding PyTorch on ARM32:

  • From what I could find/research, there are no prebuilt PyTorch wheels for ARM32 with python 3.9.
  • Yep, downgrading to python 3.7 would work.1 I was concerned with having to make sure every raspberry pi have python 3.7 since @edan-parker said that they have python 3.9 by default. I'm all for downgrading if we think managing the python versions on all the pis will be easier.
  • I really like the suggestion of making a GitHub release, and would prefer it to publishing to another third-party wheel hosting website, mainly because I have more experience working with GitHub releases.

So, the options are:

  1. Downgrade to python 3.7 and manage it on all the raspberry pis.
  2. GitHub releases of the wheel that we need to properly host it.

I'm fine either way, and would like some more opinions on the matter, especially with the new info regarding python version compatibility @jaredmcgrath.

Regarding managing architecture differences: I'm interested in looking into using sys_platform/platform_aarchitecture and wouldn't mind hashing that out to learn more about it. I'm a bit busy the rest of the week, so I'll look into it during the weekend.

Footnotes

  1. To maintain torch 1.9.0. Going with Python 3.8 would mean we'd have to settle with PyTorch 1.7. I don't know enough about the ecosystem to make that call.

@eparker-wavefin
Copy link
Collaborator

@bryan-hoang

Here's a thought - what if we downgrade to python 3.7 on the PIs for now so we can get a working solution, and we can investigate the Github releases option, if we feel that it's worth it. As well as researching the sys_architecture stuff in the future. So in the interim, we could ship this PR, and create another issue to encapsulate the other discussed points.

@bryan-hoang
Copy link
Owner Author

bryan-hoang commented Feb 10, 2022

@bryan-hoang

Here's a thought - what if we downgrade to python 3.7 on the PIs for now so we can get a working solution, and we can investigate the Github releases option, if we feel that it's worth it.

Sounds good to me.

So in the interim, we could ship this PR, and create another issue to encapsulate the other discussed points.

I'll make the corresponding issues then and leave the merging to ya. I'll start working on changing the python version dependence back to 3.7.

@bryan-hoang
Copy link
Owner Author

bryan-hoang commented Feb 10, 2022

I've tried downgrading to Python 3.7, but now axon is throwing an exception on the worker side, which hasn't occurred on Python 3.9.

$ python src/worker.py 
Exception ignored in: <function ClientSession.__del__ at 0xb58ee468>
Traceback (most recent call last):
  File "/home/bryan/.local/share/virtualenvs/mthe-493-group-a2-e9HO8nW2/lib/python3.7/site-packages/aiohttp/client.py", line 269, in __del__
    if not self.closed:
  File "/home/bryan/.local/share/virtualenvs/mthe-493-group-a2-e9HO8nW2/lib/python3.7/site-packages/aiohttp/client.py", line 894, in closed
    return self._connector is None or self._connector.closed
AttributeError: _connector
Traceback (most recent call last):
  File "src/worker.py", line 155, in <module>
    asyncio.run(main())
  File "/home/bryan/.asdf/installs/python/3.7.12/lib/python3.7/asyncio/runners.py", line 43, in run
    return loop.run_until_complete(main)
  File "/home/bryan/.asdf/installs/python/3.7.12/lib/python3.7/asyncio/base_events.py", line 587, in run_until_complete
    return future.result()
  File "src/worker.py", line 142, in main
    axon_local_ips = await discovery.broadcast_discovery(num_hosts=1, port=config.comms_config.notice_board_port)
  File "/home/bryan/.local/share/virtualenvs/mthe-493-group-a2-e9HO8nW2/lib/python3.7/site-packages/axon/discovery.py", line 25, in broadcast_discovery
    async with aiohttp.ClientSession(conn_timeout=timeout) as session:
TypeError: __init__() got an unexpected keyword argument 'conn_timeout'

I'm going to make an issue in the axon repo and see what @DuncanMays has to say about this.

Now, the wheel for pytorch is fetched from a remote url on a GitHub
repo. Also upgraded to axon 0.1.4 for a patch fix.

Closes #11
@bryan-hoang
Copy link
Owner Author

@edan-parker with b919f17 addressing your comment, this PR should be in a good state to merge.

@bryan-hoang bryan-hoang requested review from eparker-wavefin and removed request for jaredmcgrath February 11, 2022 19:43
If the system's platform is linux, adds a change of where pytorch is installed from
based on the platform's architecture.

Closes #12.
@eparker-wavefin eparker-wavefin merged commit 3ae4a95 into main Mar 14, 2022
@eparker-wavefin eparker-wavefin deleted the feature/dependency-management branch March 14, 2022 23:45
bryan-hoang pushed a commit that referenced this pull request Dec 5, 2024
Improve dependency management and testing.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve dependency management
3 participants