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

Add executorlib container #589

Merged
merged 8 commits into from
Feb 13, 2025
Merged

Add executorlib container #589

merged 8 commits into from
Feb 13, 2025

Conversation

niklassiemer
Copy link
Member

closes #584

Copy link

Binder 👈 Launch a binder notebook on branch pyiron/pyiron_workflow/executorlib01

@liamhuber
Copy link
Member

@niklassiemer, I guess this

 ======================================================================
ERROR: test_executors (integration.test_workflow.TestWorkflow.test_executors)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/pyiron_workflow/pyiron_workflow/tests/integration/test_workflow.py", line 152, in test_executors
    Workflow.create.ExecutorlibExecutor,
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'Creator' object has no attribute 'ExecutorlibExecutor'

----------------------------------------------------------------------

Just needs to be replaced with the relevant Workflow.create.excutorlib... counterpart, then we're good to go.

I suggest adopting one of the two suggestions I made to tighten up the interface, but if you hate both I can live with it as-is and otherwise once the tests are passing I'm happy here. Thanks for taking care of it!

@niklassiemer
Copy link
Member Author

niklassiemer commented Feb 12, 2025

Thanks for the feedback! I was actually using the tests here to figure out what needs to be changed in the tests xD and I was thinking to add __all__ Executors in a loop. However, I like the class attributes more :)

Indeed, I would like to have pyiron_workflow aligned to executorlib asap to do a new release and allow the cmti environment to use the newest base / atomistics/workflow stack.

#Edit
I had not seen you second review comment at the mobile at first... I love it :D

@niklassiemer
Copy link
Member Author

Funny enough all my code will be gone and you did the full work after all xD

@niklassiemer niklassiemer changed the base branch from main to dependabot/pip/executorlib-0.1.0 February 12, 2025 19:35
@niklassiemer niklassiemer changed the base branch from dependabot/pip/executorlib-0.1.0 to main February 12, 2025 19:35
@niklassiemer
Copy link
Member Author

I should not try these things on mobile xD do we want to maintain downwards compatibility to older versions of executorlib? Otherwise I would need to target the executorlib update branch I suppose :)

dependabot bot and others added 5 commits February 13, 2025 11:57
Bumps [executorlib](https://github.com/pyiron/executorlib) from 0.0.10 to 0.2.0.
- [Release notes](https://github.com/pyiron/executorlib/releases)
- [Commits](pyiron/executorlib@executorlib-0.0.10...executorlib-0.2.0)

---
updated-dependencies:
- dependency-name: executorlib
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
main code shoule be independent of executorlib version
@niklassiemer
Copy link
Member Author

@liamhuber I do not know why the coveralls/codacy is failing - is it related to some hickups (I saw a comment of Jan on another PR but I do not remember where and I am not sure if this is related).
From my perspective this is good to go otherwise. Also the API change is rather small (....create.ExecutorlibExecutor -> ....create.executorlib.Executor or ....create.executorlib.SingleNodeExecutor; depending on the version of executorlib).
If you are happy, I would like a merge and a timely new release, such that I can get this into the cmti environment on Monday.

@liamhuber liamhuber merged commit d55bb2a into main Feb 13, 2025
16 of 18 checks passed
@liamhuber liamhuber deleted the executorlib01 branch February 13, 2025 18:33
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.

Upgrade create interface to be compliant with executorlib 0.1.0
3 participants