Skip to content

Some more functionality to do things in parallel in Oscar #4780

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

Merged

Conversation

HechtiDerLachs
Copy link
Collaborator

Copy link
Collaborator

@afkafkafk13 afkafkafk13 left a comment

Choose a reason for hiding this comment

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

I just read through your PR and I am definitely interested in this functionality. I left you a few comments on the documentation -- which simply reflect what came to my mind, when reading the PR out of curiosity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you might actually want to (exceptionally) point the interested programmer to this file for a minimal working example.

@antonydellavecchia
Copy link
Collaborator

It seems that the following snippet we copied to lines to 220 to 236 is not supported in julia 1.6

  # Copied from Distributed.jl/src/workerpool.jl.
  # This puts the worker back to the pool once the future is ready
  # and we do not have to worry about this ourselves.
  local fut
  try
    fut = remotecall(f, wid, args...; kwargs...)
  catch
    put!(wp, wid)
    rethrow()
  end
  t = Threads.@spawn Threads.threadpool() try
    wait(fut)
  catch
  finally
    put!(wp, wid)
  end
  errormonitor(t)

Which appears to handle putting the workers back to the pool better, and is supported in 1.10 since the tests pass for 1.10.

and it seems that the code in Distributed/src/workerpool.jl for julia 1.6 (below), doesn't work for 1.11

   worker = take!(pool)
    try
        rc_f(f, worker, args...; kwargs...)
    finally
        put!(pool, worker)
    end

So it seems we would need to drop support for 1.6 to merge this PR, thoughts @fingolfin @benlorenz ?

@thofma
Copy link
Collaborator

thofma commented Apr 4, 2025

So it seems we would need to drop support for 1.6 to merge this PR, thoughts @fingolfin @benlorenz ?

Can't you make a branch depending on VERSION?

@antonydellavecchia
Copy link
Collaborator

So it seems we would need to drop support for 1.6 to merge this PR, thoughts @fingolfin @benlorenz ?

Can't you make a branch depending on VERSION?

it wont compile in julia 1.6

So it seems we would need to drop support for 1.6 to merge this PR, thoughts @fingolfin @benlorenz ?

Can't you make a branch depending on VERSION?

It wont compile so branching wont work

ERROR: LoadError: LoadError: LoadError: LoadError: MethodError: no method matching var"@spawn"(::LineNumberNode, ::Module, ::Expr, ::Expr)
Closest candidates are:
  var"@spawn"(::LineNumberNode, ::Module, ::Any) at threadingconstructs.jl:166

I can try and see if i can replace threadpool with something would work in both?

@thofma
Copy link
Collaborator

thofma commented Apr 4, 2025

Try putting two different methods at the toplevel using @static if VERSION <=. For example

@static if VERSION <= 1.0
  function bla(x)
  end
else
  function bla(x)
  end
end

Copy link
Member

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

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

Looks good and seems to work fine, thanks!

@antonydellavecchia antonydellavecchia merged commit ea7db71 into oscar-system:master Apr 10, 2025
32 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.

5 participants