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

[RSDK-9936] Get the CI system to build the module again #30

Merged
merged 10 commits into from
Feb 10, 2025

Conversation

penguinland
Copy link
Contributor

Tried on my Orin Nano: seems to work!

Changes include:

  • Github has upgraded its CI runners to Python 3.13. This PR upgrades pillow and numpy to py3.13-compatible versions.
  • Print more diagnostic info in the github action that installs dependencies. If the dependencies install correctly, no one will notice it, and if they don't install correctly, this will be super useful.
  • The py3.13 linter has a new check for too many positional arguments, which the Viam API fails. We need to faithfully implement the API, so I disabled that check on the line that violates it.
  • Half the tests were getting skipped without installing pytest-asyncio, so I added that in.
  • Most of requirements.txt was not pinned: you and I might be running very different versions of each dependency, because we get whatever the latest one was when we installed. This can be dangerous if one of those packages makes a breaking change. So, I pinned everything to what I installed. I'm open to other versions instead, but think it's important to pin them all.
  • I added a .gitignore so that running git status shows the repo as clean even after running the tests. Why didn't other people have this issue in the past? I wonder if I was supposed to set things up differently...
  • and I added more to .gitignore so that the repo is clean even after running make setup. I could be wrong about this: maybe you want the presence of those build artifacts front-and-center.
  • When running as a local module, viam-server expects meta.json to be in the same directory as your .tar.gz file. So, I symlinked it into dist/ in build.sh. I also forced the symlink to be created, so that it doesn't fail if you run make setup a second time. That has a bit of a smell, but I couldn't think of a better approach. Any ideas?
  • The Python SDK renamed SUBTYPE to API recently, so I renamed to match.

There's still something slightly off: make dist/archive.tar.gz failed with errors about a nonexistent file, until I ran make setup. Since then, make dist/archive.tar.gz doesn't run because everything is up-to-date even though I edited the code. but perhaps that's a problem for another day.

(I also had spurious errors about being unable to open /tmp/something/libpython3.10.so, but then they spontaneously disappeared 🤷 )

@@ -34,5 +34,7 @@ echo installing dependencies from requirements.txt
$VIRTUAL_ENV/bin/pip install -r requirements.txt -U
source $VIRTUAL_ENV/bin/activate
$PYTHON -m PyInstaller --onefile --hidden-import="googleapiclient" --add-data="./src:src" src/main.py
# When running as a local module, we need meta.json to be in the same directory as the module.
ln -sf ../meta.json dist
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line looks wrong, but it's right. It looks like "look one directory up for a file, and add a symlink in dist/" but it's actually "add to dist/ a symlink that goes one directory up from there (to the current directory).

Copy link

@bhaney bhaney left a comment

Choose a reason for hiding this comment

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

Having to include the meta.json in the dist for pyInstaller builds I think is something we've encountered before too, and seems like potentially something we didn't expect? I would want to ask people working on our module building if this is expected behavior (does meta.json need to be the PyInstaller dist) @michaellee1019

In addition to your manual test that you did, I'd like you to release an -rc version to the registry and see if it also works on your orin nano

@penguinland
Copy link
Contributor Author

include the meta.json

My understanding (which I haven't double-checked recently) is that meta.json needs to be in the directory where you run viam module upload, or the same directory as any local module. It doesn't need to be in the .tar.gz file itself when uploading to the registry.

release an -rc version to the registry

I could do this manually, but I'd prefer to merge this and #29 and then do it via github. Would that be alright?

@penguinland
Copy link
Contributor Author

We discussed in person: the release candidate can wait until after #29 is merged.

@penguinland penguinland merged commit 6601b24 into viam-modules:main Feb 10, 2025
1 check passed
@penguinland penguinland deleted the pip branch February 10, 2025 15:32
@penguinland
Copy link
Contributor Author

release an -rc version to the registry

Yes, version 0.2.9-rc1 works on my Orin Nano! I'm going to release 0.2.9 as a non-RC.

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.

2 participants