-
Notifications
You must be signed in to change notification settings - Fork 2
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
Ensure compatibility with Plasmo 0.5.4 and static partition example #5
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #5 +/- ##
=======================================
Coverage 89.13% 89.13%
=======================================
Files 4 4
Lines 313 313
=======================================
Hits 279 279
Misses 34 34 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tso-martin: Are the value
and dual
functions not working as expected when given the graph as an argument in Plasmo v0.5.4? Removing the graph argument should cause issues when running on multiple threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get errors for each thread, when value
and dual
are called with the graph argument. Example:
nested task error: KeyError: key MOI.VariableIndex(37) not found
Stacktrace:
[1] getindex
@ xxx\.julia\packages\MathOptInterface\yczX1\src\Utilities\CleverDicts.jl:164 [inlined]
[2] getindex
@ xxx\.julia\packages\MathOptInterface\yczX1\src\Utilities\copy\index_map.jl:64 [inlined]
[3] get(node_pointer::Plasmo.NodePointer, attr::MathOptInterface.VariablePrimal, idx::MathOptInterface.VariableIndex)
@ Plasmo xxx\SchwarzTryout\dev\Plasmo\src\moi_backend_node.jl:287
[4] value(graph::OptiGraph, var::VariableRef)
@ Plasmo xxx\SchwarzTryout\dev\Plasmo\src\optigraph.jl:514
[5] (::SchwarzOpt.var"#25#27"{OptiGraph})(::Pair{Int64, VariableRef})
@ SchwarzOpt .\none:0
[6] iterate(g::Base.Generator, s::Vararg{Any})
@ Base .\generator.jl:47 [inlined]
[7] _all(f::Base.var"#384#386", itr::Base.Generator{Dict{Int64, VariableRef}, SchwarzOpt.var"#25#27"{OptiGraph}}, ::Colon)
@ Base .\reduce.jl:1287
[8] all
@ .\reduce.jl:1283 [inlined]
[9] Dict(kv::Base.Generator{Dict{Int64, VariableRef}, SchwarzOpt.var"#25#27"{OptiGraph}})
@ Base .\dict.jl:111
[10] _do_iteration(subproblem_graph::OptiGraph)
@ SchwarzOpt xxx\SchwarzTryout\dev\SchwarzOpt\src\optimizer.jl:305
[11] macro expansion
@ xxx\SchwarzTryout\dev\SchwarzOpt\src\optimizer.jl:414 [inlined]
[12] (::SchwarzOpt.var"#45#threadsfor_fun#32"{SchwarzOpt.var"#45#threadsfor_fun#31#33"{…}})(tid::Int64; onethread::Bool)
@ SchwarzOpt .\threadingconstructs.jl:214
[13] #45#threadsfor_fun
@ SchwarzOpt .\threadingconstructs.jl:181 [inlined]
[14] (::Base.Threads.var"#1#2"{SchwarzOpt.var"#45#threadsfor_fun#32"{SchwarzOpt.var"#45#threadsfor_fun#31#33"{…}}, Int64})()
@ Base.Threads .\threadingconstructs.jl:153
Removing the graph argument worked fine in multiple threads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, that is definitely a bug. Without the graph argument, value
grabs the last solution the node was used in. So it's possible it takes the wrong one if there are multiple subgraphs that use it. I will take a look at this and get a fix out. Thanks for checking into this!
Hey @tso-martin; would you be able to split your example to a different pull request? In the meantime, I am fixing the current main branch of SchwarzOpt.jl to v0.5.4 and working on updating this package to work with Plasmo.jl v0.6+ which will provide a cleaner interface to work with. Edit: Actually, this seems to be an issue inside the serial implementation with |
Hi @jalving, I am a bit busy at the moment. Would 10th September be okay for you? |
@tso-martin no rush at all; i will also try to get to this soon. |
value
anddual
calls to Plasmo 0.5.4