-
Notifications
You must be signed in to change notification settings - Fork 412
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
Add Wandelbots NOVA bridge as example #9161
base: main
Are you sure you want to change the base?
Conversation
…visualization features
…sualization, trajectory playback, collision scene visualization, and continuous monitoring mode
…nore, environment variables, and project configuration
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.
Hi! Thanks for opening this pull request.
Because this is your first time contributing to this repository, make sure you've read our Contributor Guide and Code of Conduct.
…ndelbots NOVA Python example
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.
Thanks a lot for the contribution. This generally looks great and would be a nice addition to our examples. However, the requirement for a login is not ideal. The way I see it, some version of this example could fall in either or both of these categories:
- A demo of Rerun and how it might integrate with 3rd party services.
examples/python/wandelbots_nova
is the right place for that, but then it should be runnable with the absolute minimum setup, ideally just apixi run -e examples wandelbots_nova
from a fresh checkout (as are our other examples). In this context, the requirement for a logging is not acceptable. - A general purpose integration between a third party service and Rerun which aims to be used as is (or with little modifications) by users of said service. For this, we typically use a separate repository (e.g https://github.com/rerun-io/cpp-example-ros2-bridge). In this case, I guess the requirement for a login would be acceptable, given that the intended audience is already using the service.
Is there anyway you could make this example work without login, e.g. by pointing on a public endpoint/dataset, or downloading static data instead of connecting to a live service?
Appart from that, I've already included a bunch of smaller comments/nits in the review, and will do a more thorough pass when we resolve the above.
Finally, to make CI happier, I suggest you install pixi (we're currently on 0.39.0
, so please downgrade your install with pixi self-update --version 0.39.0
) and check for lints and formatting:
pixi run toml-fmt
pixi run py-lint
pixi run py-fmt
@@ -0,0 +1,2 @@ | |||
NOVA_API= | |||
NOVA_ACCESS_TOKEN= |
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.
Missing newline at EOF will be rejected by our 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.
I removed the example code as it requires login
@@ -0,0 +1 @@ | |||
models |
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.
Same
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 removed the example code as it requires login
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.
Please rename the directory wandelbots_nova
. The _rerun
part is superfluous given it's in the rerun repository :)
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.
these needs to be named wandelbots_nova.py
, since it should be a valid installable python package (no need to make a subdir + __init__.py
though, as long as it fits a single 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.
I removed the example code as it requires login
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.
Let's not leak tooling specific stuff here.
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 removed the example code as it requires login
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.
This needs to be fleshed out to be an installable pyproject.toml
, so it needs to specify a build backend. See our other examples. I would strongly suggest using hatch
for that purpose, for consistency reason.
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 removed the example code as it requires login
To use the bridge you need to install the [wandelbots-nova](https://github.com/wandelbotsgmbh/wandelbots-nova) package and access to the Wandelbots NOVA platform. | ||
|
||
Apply at: [wandelbots.com/contact](https://www.wandelbots.com/contact). |
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.
This is unideal. See main review comment.
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.
Since it is in another project should I leave this here or should I remove it and let the explanation be in https://github.com/wandelbotsgmbh/wandelbots-nova ?
Since the example is controlling a robot (virtual or real one) this will only work with a login as for now. :/ The project itself is also on Github with similar examples https://github.com/wandelbotsgmbh/wandelbots-nova |
Co-authored-by: Antoine Beyeler <49431240+abey79@users.noreply.github.com>
Correct. All of our example are based on datafile that are converted to RRD by the example itself. We will not have rrd backward compatibility until Rerun 1.0 (and our path to that will incur a lot of breaking change in the next few releases).
Readme-only examples on this repo exist only for the purpose of being listed on our landing page (which pulls data from this repo for examples and some other stuff). At this point, I'm not entirely sure that this is a good item to have there, because there is actually very little Rerun SDK content that is visible (since your tooling already implements the bridge—which is great!), and it doesn't offer a way to easily see rerun in action (since this is gated by a registration process and a login). I will double-check that with the team. |
Related
What
Added a comprehensive Nova-Rerun bridge example