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

_send_warning always raises in strict mode #3715

Closed
jleibs opened this issue Oct 5, 2023 · 6 comments
Closed

_send_warning always raises in strict mode #3715

jleibs opened this issue Oct 5, 2023 · 6 comments
Assignees
Labels
🪳 bug Something isn't working
Milestone

Comments

@jleibs
Copy link
Member

jleibs commented Oct 5, 2023

_send_warning sounds innocuous enough that we've used it in some places that maybe shouldn't be fatal for users running in strict mode.

We should rename this in a way that makes it clear that this is the behavior and consider adding a less fatal way of notifying users in cases that really should only ever be warnings.

@jleibs jleibs added the 🪳 bug Something isn't working label Oct 5, 2023
@jleibs jleibs added this to the 0.9.1 milestone Oct 5, 2023
@jleibs
Copy link
Member Author

jleibs commented Oct 5, 2023

Note, this is the cause of a reported error where AnyValue raises a TypeError if passed a None value.

@emilk
Copy link
Member

emilk commented Oct 9, 2023

I agree with the rename, but I would expect strict to always mean "throw instead of warn"

abey79 pushed a commit that referenced this issue Oct 9, 2023
### What
- Rename `_send_warning` -> `_send_warning_or_raise` to make the
behavior clearer.
 - Default to ignore Nones in AnyValues when the type is unknown. 
 - Update the docstrings.
 - Expand the unit-tests.

Resolves:
 - #3715
 - #3716


### 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/3725) (if
applicable)

- [PR Build Summary](https://build.rerun.io/pr/3725)
- [Docs
preview](https://rerun.io/preview/5540cb4172d7bbef4749daf200ee14672af010e7/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/5540cb4172d7bbef4749daf200ee14672af010e7/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
@jleibs
Copy link
Member Author

jleibs commented Oct 9, 2023

I agree with the rename, but I would expect strict to always mean "throw instead of warn"

Right now this applies only to python warnings. For example, this code in the batcher produces a warning from rust.
https://github.com/rerun-io/rerun/blob/main/crates/re_log_types/src/data_table_batcher.rs#L228

Should this raise an exception instead?

@emilk
Copy link
Member

emilk commented Oct 9, 2023

Ideally yes - if we could implement it in a simple way. We could have a log-handler that looks for warn and error logs on the current thread. But that would not work for things that we process on a background thread, e.g. batching, and it is
usually there (in background threads) that we need to log errors rather than returning them (like your linked example).

Related: https://github.com/rerun-io/rerun/blob/main/CODE_STYLE.md#error

(BTW: we already have a strict-mode in Rust that panics on warn and error logs, but that is a separate topic).

@jleibs
Copy link
Member Author

jleibs commented Oct 9, 2023

Interesting -- I'm still not fully convinced that all warnings should be panics/exceptions in strict mode, though I agree many of them probably should be.

Still, my preferred interpretation of strict mode is something along the lines of "exceptions for warnings about something that happen as a direct function of user input and for which user could practically change their code to avoid." Panicking or throwing exceptions for warnings that happen in circumstances that are outside the control of the user, such as data being dropped because a network connection went down, still seems like a bad experience.

Either way, the helper function was renamed in #3716 and I don't have a concrete warning I want to make non-exceptional at the moment, so I'm going to close for now.

@jleibs jleibs closed this as completed Oct 9, 2023
@emilk
Copy link
Member

emilk commented Oct 9, 2023

Very true, connection errors and other things outside the users control should not be affected by strict mode.

(The Rust strict mode is only for tests. All errors are already returned using Result, always)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants