Use the same executor for pyiron table and function container

[jan-janssen]

Combine the work in #1050 and #1296

Example code - for parallel pyiron table:

from pyiron_atomistics import Project
pr = Project("test")
pr.remove_jobs(silently=True, recursive=True)

job = pr.create.job.Lammps(job_name="lmp")
job.structure = pr.create.structure.ase.bulk("Al", cubic=True)
murn = job.create_job(pr.job_type.Murnaghan, "murn")
murn.run()

def filter_function(job):
    return job.__name__ == "Lammps"

table = pr.create_table()
table.filter_function = filter_function
table.add.get_energy_tot
table.add.get_volume
table.server.cores = 4
table.run(delete_existing_job=True)

table.get_dataframe()

EDIT: syntax highlight flag

[jan-janssen]

Functionality seems good, I have requested changes but I see no fundamental barriers. Non-nit requests:

• I see no need for a new python job class, just power up the main one
• JobWithExecutor.executor_type needs to be more robustly handled for early failure, and I propose a way to make it both more elegant in implementation and flexible
• Test needs to cover the failure case of a bad executor assignment

@liamhuber Thanks for your review, I tried to address the individual points in the separate comments. I just do not feel so easy to allow all possible executors, I prefer to whitelist existing executors and when somebody wants to use specific executors with pyiron_base and adds the corresponding tests I am happy to add those to the white list. Apart from this I have an intrinsic motivation to promote the use of pympipool in particular as I am currently working on improving the performance of pympipool in comparison to the built-in Process Pool executor pyiron/executorlib#264 .

[liamhuber]

One blocking comment re: turning the functionality back off and one minor comment re: UX.

Otherwise, I won't insist on these but I would encourage you to carefully consider this feedback, which is two closely related ideas:

I just do not feel so easy to allow all possible executors, I prefer to whitelist existing executors and when somebody wants to use specific executors with pyiron_base and adds the corresponding tests I am happy to add those to the white list.

(Non-blocking) Walled-garden

By adhering to a strict whitelist paradigm, you force new users to adopt the pyiron ecosystem entirely, or avoid pyiron. If a user comes along with some new executor they want to use, they need to explicitly modify pyiron_base just to check if it works.

In contrast, if we place some reasonable constraint on the value (i.e. inherits from the base concurrent.futures.Executor to guarantee basic interface), and give guidance that certain only executors are guaranteed to work, then users can experiment with new things and pull pyiron into their preferred toolset. Then, if it works, they are free to suggest that their executor be added to the test suite, etc.

IMO this is the much more pythonic approach, and avoids locking-out users with particular executor demands.

Apart from this I have an intrinsic motivation to promote the use of pympipool in particular...

(Non-blocking) Nudge don't force

This is an extremely fair perspective. I would encourage nudging users instead of forcing them to adopt a particular behaviour though. In this case, the pympipool executors should be "promoted" and "supported", so we mention them by name, have tests to guarantee they work, and lower the barrier to using them relative other executors.

This is exactly what I do over in pyiron_workflow, where Node.executor must inherit from the base executor to guarantee interface, warnings are made that executors need to support serializing dynamic stuff with docs directing users to pympipool, and then the pympipool executors are made as accessible as possible by storing them in Workflow.create.executor.

I would suggest a similar architecture here, so that pympipool holds a place of extreme privilege without being exclusive.

[jan-janssen]

@liamhuber I followed your suggestion and added the option to define executors as both strings and python classes. From my perspective this pull request is now ready to be merged.

[liamhuber]

LGTM.

I recommend extending the tests to be a bit more thorough for the three executor cases.