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

Upgrade create interface to be compliant with executorlib 0.1.0 #584

Closed
liamhuber opened this issue Feb 10, 2025 · 2 comments · Fixed by #589
Closed

Upgrade create interface to be compliant with executorlib 0.1.0 #584

liamhuber opened this issue Feb 10, 2025 · 2 comments · Fixed by #589
Labels
dependencies Pull requests that update a dependency file good first issue Good for newcomers

Comments

@liamhuber
Copy link
Member

liamhuber commented Feb 10, 2025

#583 is going to fail because we from executorlib import Executor, which is no longer available since this was decomposed into a variety of more specific executors.

I see three likely resolutions:

  1. The most direct solution is to simply swap out our import for one of these more specific classes. If we choose the right one, I suspect this will work out of the box.
  2. A slightly more elegant solution is to make a new container that imports all of the executors and gives access to them; i.e. instead of Workflow.create.ExecutorlibExecutor, we'd have something like Workflow.create.executorlib.SingleNodeExecutor or Workflow.create.executorlib.SlurmJobExecutor.
  3. The final route would be to remove the creator-access to these altogether. I still want to test against executorlib in the test suite, and probably the notebooks where we sell it, but then they wouldn't need to be in the core dependencies.

I like both (2) and (3). Since the mandatory dependencies are so light for executorlib, I would probably go for (2) as a first attack.

I don't mind this issue being on my plate, but I'm not likely to be able to get to it imminently and it should be a relatively straightforward fix, so I laid out the attack patterns here in case someone else wants to take a crack at it in the meantime.

@liamhuber liamhuber added dependencies Pull requests that update a dependency file good first issue Good for newcomers labels Feb 10, 2025
@niklassiemer
Copy link
Member

I am confused that the original PR did not fail?!

@niklassiemer
Copy link
Member

Or is this a side effect of #592 where the same commit did pass the tests (targeting another branch of course)?! This would cause quite some implications for the automerge jan developed (false test passing?!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants