Skip to content

Conversation

@ngoldbaum
Copy link
Collaborator

Fixes #53.

For reasons I don't understand, binding orig_interval in the outer function scope doesn't always work on 32 bit python.

I also took the opportunity to make sure calling sys.setswitchinterval(orig_interval) always happens after we set it to 10 microseconds in the pytest plugin using a try/finally block.

I manually verified this works on a 32 bit linux docker image from manylinux. I don't particularly want to fight with github actions to figure out how to add 32 bit CI. If anyone wants to take that on, please feel free.

Comment on lines +113 to +114
finally:
sys.setswitchinterval(original_switch)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this just skip parallel execution if the switch interval value cannot be defined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The finally block is just to make sure we always restore the original switch interval no matter what happens.

The change as a whole makes sure the binding for the original switch interval is always valid. I'm not totally sure why it's sometimes invalid on 32 bit Python. I think the code as you had it originally should have worked.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if the setswitchinterval call emits a ValueError, wouldn't the parallel execution will be skipped? Shouldn't we also enclose that instruction in a try/except statement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think a ValueError is possible now, since we're always getting a valid orig_interval before calling setswitchinterval.

If it somehow does happen there's another bug we're not considering and I'd rather fix that instead of papering over it by ignoring a valid exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry! I misread the issue completely, I thought the issue was with sys.setswitchinterval(0.000001)

Copy link
Contributor

@andfoy andfoy left a comment

Choose a reason for hiding this comment

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

Thanks @ngoldbaum !

@andfoy andfoy merged commit 8699aaa into Quansight-Labs:main Apr 28, 2025
9 checks passed
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.

7 tests fail on i586 architacture

2 participants