pyiron table: Use with-statement for executor

[jan-janssen]

No description provided.

[liamhuber]

By using the with clause, we're locking down the main process until the executor's work is complete. It's possible that this is as-intended, and that the purpose of the executor is just to provide additional computing power and not concurrency with the main process, but that surprises me and if that's the only point it needs to be clearly documented as such.

This PR also brought to my attention related issues (probably separate PRs except for removing a _):

• GenericJob.executor_type needs documentation #1386
• Why bypass the getter for GenericJob.executor_type? #1387

[jan-janssen]

By using the with clause, we're locking down the main process until the executor's work is complete. It's possible that this is as-intended, and that the purpose of the executor is just to provide additional computing power and not concurrency with the main process, but that surprises me and if that's the only point it needs to be clearly documented as such.

This is intended. The executor here is only used for parallelisation. In pyiron_base we currently still use job.server.run_mode.non_modal = True to send a job to the background. It is possible to combine both, the run_mode non_modal and setting an executor. Still the primary focus of this pull request was fixing the bug with failing tests that we had for the last three weeks, so I would merge it as is and leave the other two issues open to address them in separate pull requests.

[liamhuber]

This is intended. The executor here is only used for parallelisation. ...

Super, that's fair. Should be explained in the docstring requested in #1386, but I agree it can be a separate PR for the docstring.

Still the primary focus of this pull request was fixing the bug with failing tests that we had for the last three weeks,

This makes the tests pass, but I am still not comfortable merging it yet. PythonFunctionContainerJob.run_static also uses GenericJob._get_executor, and there it is still not wrapped in a with-clause. Yet we do test this function with an executor here and it is not causing the test failure -- why not? Isn't this test also leaving a dangling executor now?

[jan-janssen]

This makes the tests pass, but I am still not comfortable merging it yet. PythonFunctionContainerJob.run_static also uses GenericJob._get_executor, and there it is still not wrapped in a with-clause. Yet we do test this function with an executor here and it is not causing the test failure -- why not? Isn't this test also leaving a dangling executor now?

Short answer - I do not know - still I agree that using the with-statement always is a cleaner solution. To give a brief overview on how I debugged the issue. I started by executing the tests one by one in #1382 then I found that by disabling the pyiron table test on Mac OS I could fix this issue in #1383 . Afterwards I realised that a with-statement might be sufficient to get it to work. One possible solution would be that two Executors interfere with each other, so the second one causes this trouble.

[liamhuber]

Short answer - I do not know

That's totally fair. But then I don't want to merge something that makes the little red flag go away. Taking the thermometer out of our mouth might make the temperature reading go down, but it doesn't cure the fever.

One possible solution would be that two Executors interfere with each other, so the second one causes this trouble.

That sounds plausible and testable!

[jan-janssen]

This makes the tests pass, but I am still not comfortable merging it yet.

This is the part I do not understand - so you prefer a broken test which prevents us from seeing issues that new pull requests introduce, rather than merging a solution which fixes the test and then spend more time on debugging it. For me the highest priority is having a working test system and after I see that nobody seem to care during the two weeks I was on conferences I sat down to fix it yesterday.

[jan-janssen]

Update

I was wrong - the if-statement does not fix the issue, it was just working once. Still I think this syntax is more clean, so I would prefer to merge this pull request anyway.

[liamhuber]

I was wrong - the if-statement does not fix the issue, it was just working once. Still I think this syntax is more clean, so I would prefer to merge this pull request anyway.

Amusingly enough, I'm actually much more OK with this PR if it doesn't fix the issue.

I would prefer not to merge failing PRs, but that ship has sailed (and I even helmed it before). However, if the purpose of the PR is the improved syntax, I'd like to see the new syntax in both use cases at once since it's the same idea both times.

[liamhuber]

I was wrong - the if-statement does not fix the issue, it was just working once. Still I think this syntax is more clean, so I would prefer to merge this pull request anyway.

Amusingly enough, I'm actually much more OK with this PR if it doesn't fix the issue.

I would prefer not to merge failing PRs, but that ship has sailed (and I even helmed it before). However, if the purpose of the PR is the improved syntax, I'd like to see the new syntax in both use cases at once since it's the same idea both times.

Nm, looks like that already happened since last time I looked at the diff

[liamhuber]

With the caveat that #1386 needs to be solved separately so it isn't weird to have an executor holding up the process, lgtm!