-
Notifications
You must be signed in to change notification settings - Fork 54
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-9623: Add Discover Service and GetModelsFromModules to Python #827
base: main
Are you sure you want to change the base?
Conversation
76b697e
to
574ade1
Compare
0dac527
to
e1832aa
Compare
@@ -16,7 +16,7 @@ buf: clean | |||
rm -rf src/viam/gen | |||
chmod +x plugin/main.py | |||
uv pip install protoletariat | |||
uv pip install protobuf==5.29.1 | |||
uv pip install protobuf==5.29.3 |
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.
want to call attention to this upgrade and make sure there's no issue with it
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.
I think it's probably fine but I haven't looked into it deeply. Curious though why this upgrade is important?
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.
with 5.29.1, when I run make buf
locally I get the following error
Detected incompatible Protobuf Gencode/Runtime versions when loading common/v1/common.proto: gencode 5.29.3 runtime 5.29.1. Runtime version cannot be older than the linked gencode version. See Protobuf version guarantees at https://protobuf.dev/support/cross-version-runtime-guarantee.
and I don't remember what the exact error was on GitHub but the checks did not pass either, I assume because the correct protos weren't present since I couldn't build them? so I upgraded protobuf and was able to run make buf
successfully, and then the GitHub checks passed
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.
Generally looks good to me! but wanna get a little bit more clarity on the proto version bump
@@ -16,7 +16,7 @@ buf: clean | |||
rm -rf src/viam/gen | |||
chmod +x plugin/main.py | |||
uv pip install protoletariat | |||
uv pip install protobuf==5.29.1 | |||
uv pip install protobuf==5.29.3 |
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.
I think it's probably fine but I haven't looked into it deeply. Curious though why this upgrade is important?
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.
Can you merge with main to make sure everything is up to date, including the protos?
yes I will! but I discovered this morning that |
okay issue resolved, I thought it was an sdk issue but it was unimplemented in rdk, works now that the implementation has been merged. and merged main so it should be updated with latest @njooma |
add
discoverResources
andgetModelsFromModules
to python sdk, deprecation warnings are already added in #818