Conversation
FabianHofmann
left a comment
FabianHofmann
left a comment
There was a problem hiding this comment.
@KristijanFaust-OET this is super powerful. and sorry again for taking so long...
in general the structure looks really good. I am just wondering about one thing:
can't we keep the generic remote argument in the solve function and allow a OetcHandler to be passed? should I make a proposal for that?
also I would like to move oetc.py into a more generic directory, like remote/ or services/ what do you think?
If I get you right, the first proposal is to construct the remote handler already on the client/pypsa in addition to parsing the settings here: https://github.com/open-energy-transition/pypsa-eur/pull/50/files#diff-12c68fdd6dfc952a2866eab1e18c44a36fb668d4fe9887e60fac39089cda8d8fL1332-L1333 Sure we can do that. I avoided coupling in any way the ssh and oetc implementation, since:
If it is not a problem, I would rather do that change in a separate PR after the merge of this one, because we already sent the pypsa client modification to Agora with this approach in mind. In order not to complicate things with them, they can remain dependant on this branch of linopy, while we do the modification on a separate branch so that they don't need to adjust anything. Regarding the folder proposal, that makes completely sense. I went with a flat hierarchy since there weren't any folders up until this point. Should we also move then the remote.py file to that folder too? |
linopy/model.py
Outdated
| if remote or oetc_settings: | ||
| if remote and oetc_settings: | ||
| raise ValueError("Remote and OETC can't be active at the same time") | ||
| if remote: | ||
| solved = remote.solve_on_remote( | ||
| self, | ||
| solver_name=solver_name, | ||
| io_api=io_api, | ||
| problem_fn=problem_fn, | ||
| solution_fn=solution_fn, | ||
| log_fn=log_fn, | ||
| basis_fn=basis_fn, | ||
| warmstart_fn=warmstart_fn, | ||
| keep_files=keep_files, | ||
| sanitize_zeros=sanitize_zeros, | ||
| **solver_options, | ||
| ) | ||
| else: | ||
| solved = OetcHandler(oetc_settings).solve_on_oetc(self) # type: ignore |
There was a problem hiding this comment.
@KristijanFaust-OET I rather meant here, if we could do something like:
| if remote or oetc_settings: | |
| if remote and oetc_settings: | |
| raise ValueError("Remote and OETC can't be active at the same time") | |
| if remote: | |
| solved = remote.solve_on_remote( | |
| self, | |
| solver_name=solver_name, | |
| io_api=io_api, | |
| problem_fn=problem_fn, | |
| solution_fn=solution_fn, | |
| log_fn=log_fn, | |
| basis_fn=basis_fn, | |
| warmstart_fn=warmstart_fn, | |
| keep_files=keep_files, | |
| sanitize_zeros=sanitize_zeros, | |
| **solver_options, | |
| ) | |
| else: | |
| solved = OetcHandler(oetc_settings).solve_on_oetc(self) # type: ignore | |
| if remote is not None: | |
| if isinstance(remote, OetHandler): | |
| solved = OetcHandler(oetc_settings).solve_on_oetc(self) # type: ignore | |
| else: | |
| solved = remote.solve_on_remote( | |
| self, | |
| solver_name=solver_name, | |
| io_api=io_api, | |
| problem_fn=problem_fn, | |
| solution_fn=solution_fn, | |
| log_fn=log_fn, | |
| basis_fn=basis_fn, | |
| warmstart_fn=warmstart_fn, | |
| keep_files=keep_files, | |
| sanitize_zeros=sanitize_zeros, | |
| **solver_options, | |
| ) |
that would enrich the remote argument. does that make sense?
There was a problem hiding this comment.
Yes exactly, but that also requires all the changes I mentioned in order to work.
Maybe I'm missing something but I don't see a pre-step in linopy where the remote argument is constructed from the config settings before being passed to the solve function. So I guess it is expected from the client to construct that, right?
There was a problem hiding this comment.
ah yes! now I understand where you are coming from! how about we keep the oetc settings in the config in pypsa-eur and if non-empty a OetcHandler is created before the solve function, so in pypsa-eur this would mean
oetc = solving.get("oetc", None)
if oetc:
oetc["credentials"] = OetcCredentials(
email=os.environ["OETC_EMAIL"], password=os.environ["OETC_PASSWORD"]
)
oetc["solver"] = kwargs["solver_name"]
oetc["solver_options"] = kwargs["solver_options"]
oetc_settings = OetcSettings(**oetc)
oetc_handler = OetcHandler(oetc_settings)
kwargs["remote"] = oetc_handler
Does that make sense?
I would like to keep the argument in the solve function abstract from oetc. I am also happy to introduce another argument like service/cloud_solver or so. But note that from a user perspective remote and oetc do a similar thing, they take the model and send it to a server to solve it there, even though the underlying architecture is very different.
There was a problem hiding this comment.
These changes are exactly what I'm talking about. But if we do this now on this branch we will break compatibility with Agora's implementation which is already shipped to them.
@siddharth-krishna
Is it okay if we do the update now, notify Agora and send them an update?
There was a problem hiding this comment.
I see, if it is already in use, I don't want to be an extra hurdle (given the ages it took until the review). we could also come up with a follow up and a proper deprecation if you want
There was a problem hiding this comment.
The change is easy to do, and I think it is not worth to have multiple branches/PRs for it, if we wouldn't have Agora in the mix. Lets wait to see what @siddharth-krishna, if he is ok with it, I'll do them in this PR, and we'll update Agora's codebase too.
KristijanFaust-OET
force-pushed
the
oetc-support
branch
from
9291606 to
5429c8d
Compare
FabianHofmann
left a comment
FabianHofmann
left a comment
There was a problem hiding this comment.
Like that! tiny comment. should we merge into upstream linopy soon?
linopy/model.py
Outdated
| oetc_settings : dict, optional | ||
| Settings for the solving on the OETC platform. If a value is provided | ||
| solving will be attempted on OETC, otherwise it will be done locally. |
There was a problem hiding this comment.
this one is not need anymore right?
Thanks for the review. I see no reason not to. @siddharth-krishna WDYT? |
|
Thank you both! Yes please, let's make the PR to upstream and merge it in ⚡ |
3 participants
Changes proposed in this Pull Request
Checklist
doc.doc/release_notes.rstof the upcoming release is included.