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

refactor(threads): error monitor spawned threads #32

Merged

Conversation

zsoerenm
Copy link
Contributor

@zsoerenm zsoerenm commented Sep 5, 2023

Spawned threads should be error monitored, because any error that is thrown inside the thread will silently be discarded otherwise.

@bvdmitri
Copy link
Member

bvdmitri commented Sep 6, 2023

Hey @zsoerenm ! Thanks, I think the change is reasonable, however, the Base.errormonitor is not available for example in Julia 1.6, so the new code might fail on the old Julia versions. This is something we wish to avoid. Could you please double-check which version introduced the Base.errormonitor and add a @static if to prevent using it on the older versions of Julia.

@bvdmitri bvdmitri added the enhancement New feature or request label Sep 6, 2023
@bvdmitri
Copy link
Member

bvdmitri commented Sep 6, 2023

Here is the list of supported versions https://github.com/biaslab/Rocket.jl/actions/runs/5998363534

Spawned threads should be error monitor, because any error that is thrown
inside the thread will silently be discarded otherwise.
@zsoerenm zsoerenm force-pushed the ss/add-errormonitor-to-spawn branch from 63b097d to cf79eab Compare September 6, 2023 20:48
@zsoerenm
Copy link
Contributor Author

zsoerenm commented Sep 6, 2023

Good point. I added a static version check.
It was added with v1.7 (see JuliaLang/julia@754aada)

Copy link
Member

@bvdmitri bvdmitri left a comment

Choose a reason for hiding this comment

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

Thanks!

@bvdmitri bvdmitri merged commit 1542205 into ReactiveBayes:master Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants