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

face_tracking example fails with new AnnotationContext implementation #2924

Closed
abey79 opened this issue Aug 7, 2023 · 5 comments · Fixed by #3017
Closed

face_tracking example fails with new AnnotationContext implementation #2924

abey79 opened this issue Aug 7, 2023 · 5 comments · Fixed by #3017
Labels
🪳 bug Something isn't working 🐍 Python API Python logging API 🦟 regression A thing that used to work in an earlier release
Milestone

Comments

@abey79
Copy link
Member

abey79 commented Aug 7, 2023

Describe the bug

It looks like rr.ClassDescription() now requires and argument that wasn't formerly required.

❯ python examples/python/face_tracking/main.py 
INFO: Created TensorFlow Lite XNNPACK delegate for CPU.
Traceback (most recent call last):
  File "/Users/hhip/src/rerun/examples/python/face_tracking/main.py", line 428, in <module>
    main()
  File "/Users/hhip/src/rerun/examples/python/face_tracking/main.py", line 422, in main
    run_from_video_capture(args.camera, args.max_dim, args.max_frame, args.num_faces)
  File "/Users/hhip/src/rerun/examples/python/face_tracking/main.py", line 311, in run_from_video_capture
    detector = FaceDetectorLogger(video_mode=True)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/hhip/src/rerun/examples/python/face_tracking/main.py", line 119, in __init__
    rr.ClassDescription(keypoint_connections=[(0, 1), (1, 2), (2, 0), (2, 3), (0, 4), (1, 5)]),
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/hhip/src/rerun/rerun_py/rerun_sdk/rerun/_rerun2/datatypes/class_description.py", line 53, in __init__
    classdescription_init(self, *args, **kwargs)
TypeError: classdescription_init() missing 1 required positional argument: 'info'
@abey79 abey79 added 🪳 bug Something isn't working 🐍 Python API Python logging API 🦟 regression A thing that used to work in an earlier release labels Aug 7, 2023
@abey79
Copy link
Member Author

abey79 commented Aug 7, 2023

@emilk
Copy link
Member

emilk commented Aug 8, 2023

why isn't mypy catching this?

@emilk emilk added this to the 0.8.1 milestone Aug 8, 2023
@abey79
Copy link
Member Author

abey79 commented Aug 8, 2023

why isn't mypy catching this?

Custom init overrides are untyped unfortunately. That's one of the major eyesore of the codegen's python SDK in its current state.

@emilk
Copy link
Member

emilk commented Aug 17, 2023

I believe this was introduced in https://github.com/rerun-io/rerun/pull/2895/files#diff-e4f8d37313c7297c2faac15acce4fa15501d41993f79b717e817213f8c20764eR30

struct ClassDescription has a pub info: crate::datatypes::AnnotationInfo, which before used the AnnotationInfo::default():

#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub struct AnnotationInfo {
    /// `ClassId` or `KeypointId` to which this annotation info belongs.
    pub id: u16,

    /// The label that will be shown in the UI.
    pub label: Option<crate::datatypes::Label>,

    /// The color that will be applied to the annotated entity.
    pub color: Option<crate::datatypes::Color>,
}

Our new code-gened ClassDescription (part of the annotation context) now require the user to supply an AnnotationInfo whereas before I believe this was set to AnnotationInfo::default if omitted (giving an id=0). I’m not sure what should be the behavior here, as I find the whole annotation context very complicated 😬

Does it make sense to have AnnotationInfo::default, or is it a footgun?
Is examples/python/face_tracking/main.py wrong not to include an info? If it should be included, what should it be set to?

Ideally I would like to give info a default value in rerun_py/rerun_sdk/rerun/_rerun2/datatypes/_overrides/annotation_context.py, but I can't because AnnotationInfoLike requires a label and a color, even though those fields are nullable.

Is attr.python.aliases in crates/re_types/definitions/rerun/datatypes/annotation_info.fbs wrong? It seems to force users to select a label and color, even though they are nullable. But perhaps it is the codegen that is wrong, missing the nullabel in this case?

emilk added a commit that referenced this issue Aug 17, 2023
* Closes #2924

This took me over an hour to figure out 😭
emilk added a commit that referenced this issue Aug 17, 2023
* Closes #2924

Two changes:
* You can now easily specify the `info` without having to also specify
label and color, using `info=0`.
* The default info is one with `id=0`, matching 0.7.0 behavior and how
`log_points` work


(This took me over an hour to figure out 😭)

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3017) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/3017)
- [Docs
preview](https://rerun.io/preview/pr%3Aemilk%2Ffix-annotation-context-default/docs)
- [Examples
preview](https://rerun.io/preview/pr%3Aemilk%2Ffix-annotation-context-default/examples)
@emilk
Copy link
Member

emilk commented Aug 17, 2023

This is not a problem on 0.8.0, so no need for a fix for 0.8.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 🐍 Python API Python logging API 🦟 regression A thing that used to work in an earlier release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants