Make UfuncHelpers thread-safe #11226

bmerry · 2021-01-08T10:54:04Z

Description

Remove modules property that lazily creates the underlying
_modules attribute, in favour of an __init__ that creates
modules as an attribute.
Add a lock that is held for all mutations of the object.
Moved UNSUPPORTED from a class attribute to an instance attribute,
for consistency. This is a singleton class and it's accessed via
the UNSUPPORTED_UFUNCS global so this shouldn't make any difference
other than making it easier to isolate unit tests.

bmerry · 2021-01-08T10:55:23Z

I wouldn't merge this exactly as is, because I copied-and-pasted a fixture from #11224. Once that is merged it should probably go to a shared location - I'm not sure what that should be.

mhvk

@bmerry - and thanks a third time! This looks good too. I was a bit hesitant about adding complexity, but in the end this is pretty deep in the bowels and central to quantities, so it seems well worth it. And the comments will help anyone looking in the future.

I'll approve now but will wait with merging until the other PRs have had a few more eyes, and we've decided on whether or not to backport.

astropy/units/quantity_helper/converters.py

pllim · 2021-01-11T23:48:43Z

astropy/units/quantity_helper/converters.py


 UFUNC_HELPERS = UfuncHelpers()
-UNSUPPORTED_UFUNCS = UFUNC_HELPERS.UNSUPPORTED
+UNSUPPORTED_UFUNCS = UFUNC_HELPERS.unsupported


Will replacing UNSUPPORTED with lowercase version break people's code? Do people use UNSUPPORTED from this class directly? Perhaps it is a smaller price to pay to break some PEP 8 and do a self.UNSUPPORTED |= ... and such. 💭

You're right, I'd missed that users could directly access it as UFUNC_HELPERS.UNSUPPORTED or UfuncHelpers.UNSUPPORTED rather than through this alias, even though all the astropy code uses the alias. I'll change it back to the way it was before (uppercase, class attribute) and think up some way to isolate it in the test.

pllim · 2021-01-11T23:51:08Z

astropy/units/tests/test_quantity_ufuncs.py

+
+    This makes it easier to provoke race conditions.
+
+    TODO: this is copied from another branch which adds it elsewhere. It


Well... Should we work to get that other PR merged first and then rebase this one on top of upstream master after that is merged? If so, please turn this PR into draft status and put a note on the top post to indicate that this PR is dependent on that other PR. (GitHub still has no proper linking like JIRA.)

But that said, is a new fixture really necessary? I can we use setup_class and teardown_class instead within TestUfuncHelpers?

#11224 is merged, so this is moot now. Please rebase and use your fixture from conftest.py. Thanks! 😸

I've rebased - the only difference should be that the fast_thread_switching fixture is gone as it was added by #11224.

mhvk

Some small wonderings. Not essential, but let me know what you think.

mhvk · 2021-01-13T14:58:27Z

astropy/units/tests/test_quantity_ufuncs.py

+        with concurrent.futures.ThreadPoolExecutor(max_workers=workers) as executor:
+            for p in range(10000):
+                helpers = UfuncHelpers()
+                # UNSUPPORTED is (for some reason) a class attribute rather than an


Hmm, ~~what~~ was this solved by your lower-case version? Sorry to only realize now, but it would seem to make sense to just initialize that in __init__ as well.

Yes, that's why I moved it from class attribute to instance attribute. I moved it back due to concern that it would break code that uses UfuncHelpers.UNSUPPORTED, although I've just searched Github and not found any hits for "UfuncHelpers" outside of astropy source code so it may be an unwarranted concern.

Looking at it again, is the UfuncHelpers name considered to be public API? It doesn't have an underscore, but it's also not listed in __all__.

I'll defer to @mhvk here for a design decision. While you couldn't find usage on GitHub, doesn't mean people are not using it in their private code. But if we do decide to make break (pun?) for this, then we'll find out soon enough if people complain about it after the next release.

My suggestion would be to leave it as upper case, but make it an instance property. Then, if anybody uses UFUNC_HELPERS.UNSUPPORTED, it will still work fine.

p.s. So a bit in between - I don't think it is likely anybody would use the class, while I think it is quite possible they might have used the instance.

Ok, I'll change it to an instance attribute tomorrow.

astropy/units/tests/test_quantity_ufuncs.py

It currently fails, because I haven't fixed the issue yet.

- Remove `modules` property that lazily creates the underlying `_modules` attribute, in favour of an `__init__` that creates `modules` as an attribute. - Add a lock that is held for all mutations of the object. Fixes astropy#11220.

It should be possible to test, but added the pragma to avoid codecov failure for code that I've merely re-indented. Co-authored-by: P. L. Lim <[email protected]>

bmerry · 2021-01-14T07:48:50Z

Ops, looks like there is a conflict in the change log too. grimacing

I've rebased to fix that. Let me know what you want to do about UNSUPPORTED (class or instance attribute).

This makes it a bit easier to test UfuncHelper without affecting the main singleton instance.

mhvk

Super, thanks so much!

github-actions bot added the units label Jan 8, 2021

This comment has been minimized.

Sign in to view

mhvk approved these changes Jan 8, 2021

View reviewed changes

mhvk added this to the v4.3 milestone Jan 8, 2021

mhvk added Bug Refactoring labels Jan 8, 2021

pllim reviewed Jan 11, 2021

View reviewed changes

pllim mentioned this pull request Jan 12, 2021

Make classproperty and lazyproperty thread-safe #11224

Merged

bmerry marked this pull request as draft January 12, 2021 10:00

pllim mentioned this pull request Jan 12, 2021

What's New entry for threadsafety improvements #11241

Merged

4 tasks

bmerry force-pushed the issue-11220 branch from adcbca2 to 94f306d Compare January 13, 2021 07:42

bmerry marked this pull request as ready for review January 13, 2021 07:44

mhvk reviewed Jan 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

bmerry and others added 3 commits January 14, 2021 09:47

Add a regression test for astropy#11220

bcfbcc7

It currently fails, because I haven't fixed the issue yet.

Make UfuncHelpers thread-safe

a945078

- Remove `modules` property that lazily creates the underlying `_modules` attribute, in favour of an `__init__` that creates `modules` as an attribute. - Add a lock that is held for all mutations of the object. Fixes astropy#11220.

Add pragma: no cover for untested code

232931b

It should be possible to test, but added the pragma to avoid codecov failure for code that I've merely re-indented. Co-authored-by: P. L. Lim <[email protected]>

bmerry force-pushed the issue-11220 branch from 94f306d to 232931b Compare January 14, 2021 07:48

Move UfuncHelper.UNSUPPORTED to an instance attribute

f5a4b6f

This makes it a bit easier to test UfuncHelper without affecting the main singleton instance.

mhvk approved these changes Jan 14, 2021

View reviewed changes

mhvk merged commit 6bf697e into astropy:master Jan 14, 2021

pllim mentioned this pull request Nov 23, 2021

TST: Update Python versions in CI #12522

Merged

9 tasks


		This makes it easier to provoke race conditions.

		TODO: this is copied from another branch which adds it elsewhere. It

Uh oh!

Make UfuncHelpers thread-safe #11226

Make UfuncHelpers thread-safe #11226

Uh oh!

Conversation

bmerry commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

bmerry commented Jan 8, 2021

Uh oh!

This comment has been minimized.

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

bmerry commented Jan 14, 2021

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bmerry commented Jan 8, 2021 •

edited

Loading

mhvk Jan 13, 2021 •

edited

Loading