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

rr.log_file(rec=rec) results in a type error #8167

Closed
EtaLoop opened this issue Nov 18, 2024 · 5 comments · Fixed by #8184
Closed

rr.log_file(rec=rec) results in a type error #8167

EtaLoop opened this issue Nov 18, 2024 · 5 comments · Fixed by #8184
Labels
🪳 bug Something isn't working 🐍 Python API Python logging API

Comments

@EtaLoop
Copy link
Contributor

EtaLoop commented Nov 18, 2024

With this code :

rec = rr.new_recording(application_id="My window", recording_id="My rec", spawn=True)
rr.log_file_from_path(file_path="file.stl", recording=rec)

I get the following error :

RerunWarning: log_file_from_path: TypeError(argument 'recording': 'RecordingStream' object cannot be converted to 'PyRecordingStream')
  rr.log_file_from_path(file_path="file.stl", recording=rec)

Did I miss something with log_file_from_path ?

@EtaLoop EtaLoop added 👀 needs triage This issue needs to be triaged by the Rerun team 🪳 bug Something isn't working labels Nov 18, 2024
@teh-cmc teh-cmc added 🐍 Python API Python logging API and removed 👀 needs triage This issue needs to be triaged by the Rerun team labels Nov 19, 2024
@teh-cmc
Copy link
Member

teh-cmc commented Nov 19, 2024

There's a small type definition bug in the Python SDK.

Until we ship a fix, the workaround is simply to call RecordingStream.to_native():

rec = rr.new_recording(application_id="My window", recording_id="My rec", spawn=True)
- rr.log_file_from_path(file_path="file.stl", recording=rec)
+ rr.log_file_from_path(file_path="file.stl", recording=rec.to_native())

@qianyizhang
Copy link

There's a small type definition bug in the Python SDK.

Until we ship a fix, the workaround is simply to call RecordingStream.to_native():

rec = rr.new_recording(application_id="My window", recording_id="My rec", spawn=True)
- rr.log_file_from_path(file_path="file.stl", recording=rec)
+ rr.log_file_from_path(file_path="file.stl", recording=rec.to_native())

this still doesn't log to desired recording in case of serve then connect.

@qianyizhang
Copy link

to me more specific:

  1. rerun --serve in cli
  2. make some local rrds
import rerun as rr
NUM_CASE = 3
path_list = []
for index in range(1, 1+NUM_CASE):
    case_id = f"case{index}"
    rr.init("demo", recording_id=case_id)
    local_path = f"./{case_id}.rrd"
    rr.save(local_path)
    path_list.append(local_path)
    rr.log(
        "triangle{case_id}",
        rr.Mesh3D(
            vertex_positions=[[0.0, index, 0.0], [index, 0.0, 0.0], [0.0, 0.0, 0.0]],
            vertex_normals=[0.0, 0.0, 1.0],
            vertex_colors=[[0, 0, 255], [0, 255, 0], [255, 0, 0]],
            triangle_indices=[2, 1, 0],
        ),
    )
  1. natively log_file_from_path works as expected
import rerun as rr
rr.init("demo", recording_id="does not matter")
rr.connect()
for path in path_list:
    rr.log_file_from_path(path) # log to pre-saved recordings, eg. case1-3
  1. log_file_from_path with specified recording doesn't work
import rerun as rr
rr.init("demo", recording_id="new_record")
rr.connect()
new_recording = rr.get_global_data_recording()
print(new_recording .get_recording_id()) # new_record
for path in path_list:
    rr.log_file_from_path(path, recording=new_recording.to_native()) # expect to add this to current recording, but still ends up in pre-saved recordings, eg. case1-3

@teh-cmc
Copy link
Member

teh-cmc commented Nov 20, 2024

👍 Let's create a separate issue for this:

@teh-cmc teh-cmc changed the title Error with log_file_from_path when specifying recording rr.log_file(rec=rec) results in a type error Nov 20, 2024
@EtaLoop
Copy link
Contributor Author

EtaLoop commented Nov 20, 2024

Thanks, this works now !

@jleibs jleibs closed this as completed in d1b80ae Nov 25, 2024
emilk pushed a commit that referenced this issue Nov 25, 2024
* Fixes #8167

It is unapologetically disgusting, but at least it works... Someone can
come up with a better solution later, I hope?

### 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 the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/8184?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/8184?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/8184)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.

To deploy documentation changes immediately after merging this PR, add
the `deploy docs` label.
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants