8000 [WIP] MAINT: Wrap ``printoptions`` in ``ContextVar`` by mtsokol · Pull Request #26345 · numpy/numpy · GitHub
[go: up one dir, main page]

Skip to content

[WIP] MAINT: Wrap printoptions in ContextVar #26345

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

Closed
wants to merge 1 commit into from

Conversation

mtsokol
Copy link
Member
@mtsokol mtsokol commented Apr 25, 2024

Issue #26314

Hi @seberg @ngoldbaum,

This (WIP) PR wraps printoptions in ContextVar. For now I tried with wrapping whole dict into a one ContextVar to see how the implementation changes - I think I can also clean/refactor the file/functions a bit (e.g. set_string_function was deprecated in 2.0, maybe we can remove it in 2.1?)

@mtsokol mtsokol self-assigned this Apr 25, 2024
@mtsokol mtsokol force-pushed the printoptions-contextvar branch from e6c3ab5 to f932b7f Compare April 25, 2024 08:19
@@ -1443,10 +1472,11 @@ def _void_scalar_to_string(x, is_repr=True):
scalartypes.c.src code, and is placed here because it uses the elementwise
formatters defined above.
"""
options = _format_options.copy()
options = _format_options.get().copy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it makes sense, but I was half hoping we can just stop passing options around completely and enter a new context ourselves when necessary. contextvar lookup is almost as fast as dict lookup, and global lookup also not really expensive at the function level (within a loop it might be).

@charris charris changed the title [WIP] MAINT: Wrap printoptions in ContextVar [WIP] MAINT: Wrap printoptions in ContextVar Apr 25, 2024
@mattip
Copy link
Member
mattip commented Jul 3, 2024

ping @mtsokol

@ngoldbaum
Copy link
Member

I'm going to take this over from @mtsokol and also deal with the legacy_print_mode C static variable.

@ngoldbaum
Copy link
Member

Closing in favor of a new PR closer to the top of the queue.

@ngoldbaum ngoldbaum closed this Jul 3, 2024
seberg pushed a commit that referenced this pull request Jul 22, 2024
This is a re-do of gh-26345, I'm taking over from @mtsokol because this is needed for the free-threaded work.

The new _printoptions.py file exists to avoid a circular import during setup of the multiarray module.

I'm guessing this adds some overhead to printing. I haven't benchmarked it because it wasn't clear to me: do we care about printing performance? I could certainly add some caching or a way to avoid repeatedly calling get_legacy_print_mode for every printed value. We could also keep the C global we had before but make it thread-local. I just thought it made things conceptually simpler to store all the printoptions state in the context variable.

Co-authored-by: Mateusz Sokół <mat646@gmail.com>
ArvidJB pushed a commit to ArvidJB/numpy that referenced this pull request Nov 1, 2024
This is a re-do of numpygh-26345, I'm taking over from @mtsokol because this is needed for the free-threaded work.

The new _printoptions.py file exists to avoid a circular import during setup of the multiarray module.

I'm guessing this adds some overhead to printing. I haven't benchmarked it because it wasn't clear to me: do we care about printing performance? I could certainly add some caching or a way to avoid repeatedly calling get_legacy_print_mode for every printed value. We could also keep the C global we had before but make it thread-local. I just thought it made things conceptually simpler to store all the printoptions state in the context variable.

Co-authored-by: Mateusz Sokół <mat646@gmail.com>
@mtsokol mtsokol deleted the printoptions-contextvar branch January 2, 2025 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0