Skip to content

Bump executorlib#817

Merged
liamhuber merged 4 commits intoshutdown-recursionfrom
bump-executorlib
Mar 3, 2026
Merged

Bump executorlib#817
liamhuber merged 4 commits intoshutdown-recursionfrom
bump-executorlib

Conversation

@liamhuber
Copy link
Member

No description provided.

Signed-off-by: liamhuber <liamhuber@greyhavensolutions.com>
@github-actions
Copy link

github-actions bot commented Mar 2, 2026

Binder 👈 Launch a binder notebook on branch pyiron/pyiron_workflow/bump-executorlib

@liamhuber
Copy link
Member Author

Coverage tests hang. The same thing happens on my local machine, where it's easy to see that it hangs at integration.test_wrapped_executorlib.TestWrappedExecutorlib.test_cache. I don't currently know why.

@jan-janssen
Copy link
Member

Coverage tests hang. The same thing happens on my local machine, where it's easy to see that it hangs at integration.test_wrapped_executorlib.TestWrappedExecutorlib.test_cache. I don't currently know why.

Do you see which of the two executors causes the hanging? NodeSingleExecutor or _CacheTestClusterExecutor ?

@jan-janssen
Copy link
Member

One thing that might be related is that in the pydantic validation of the resource_dict, the cache_directory variable was not available before, basically because I thought it would be sufficient to only specify the cache_key. But I remember we talked about the option to add a cache_directory in the resource_dict as well, so this is added again in pyiron/executorlib#945 .

@liamhuber
Copy link
Member Author

Do you see which of the two executors causes the hanging? NodeSingleExecutor or _CacheTestClusterExecutor ?

Unfortunately, no. Since it doesn't actually fail all I see is the parent test name. I will try splitting the subtest up into two separate tests and hopefully that will reveal. Answer follows at end.

One thing that might be related is that in the pydantic validation of the resource_dict, the cache_directory variable was not available before, basically because I thought it would be sufficient to only specify the cache_key. But I remember we talked about the option to add a cache_directory in the resource_dict as well, so this is added again in pyiron/executorlib#945 .

Stellar. I checked out that branch and re-ran the tests locally, and the situation is improved. I have some lingering fear because the first time I ran my tests locally they all completed successfully, but the process hung afterwards and wouldn't shut down the test suite. But I've since re-run the whole test suite and the problematic tests directly (with attempts from both the file- and function-levels) and can't reproduce the problem.

Aha, ok, splitting up and running from the function level reveals that the _CacheTestClusterExecutor gives a warning:

/Users/liamhuber/dev/miniforge3/envs/pyiron12/lib/python3.12/subprocess.py:1127: ResourceWarning: subprocess 94559 is still running
  _warn("subprocess %s is still running" % self.pid,
ResourceWarning: Enable tracemalloc to get the object allocation traceback

My gut reaction is that this is probably the thread I start to shut down the executor? In any case, I can't get it to reproduce a problem beyond the warning level.

@jan-janssen
Copy link
Member

@liamhuber just for confirmation, the changes to executorlib fix the issue, correct? Then I merge those and release 1.9.1

@liamhuber
Copy link
Member Author

liamhuber commented Mar 3, 2026

Confirmed. There's a disconcerting warning, but at least on my local machine that PR gets all the tests working well. 1.9.1 would be appreciated; I'll bump to it tomorrow (pacific)

@codecov
Copy link

codecov bot commented Mar 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.36%. Comparing base (38009e1) to head (c23a014).
⚠️ Report is 5 commits behind head on shutdown-recursion.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           shutdown-recursion     #817      +/-   ##
======================================================
- Coverage               93.46%   93.36%   -0.11%     
======================================================
  Files                      38       38              
  Lines                    3873     3873              
======================================================
- Hits                     3620     3616       -4     
- Misses                    253      257       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@jan-janssen jan-janssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liamhuber I updated executorlib to 1.9.1 and all tests pass - so from my perspective it’s ready to be merged.

@liamhuber
Copy link
Member Author

@liamhuber I updated executorlib to 1.9.1 and all tests pass - so from my perspective it’s ready to be merged.

Agreed 💯

@liamhuber liamhuber merged commit 05bd76f into shutdown-recursion Mar 3, 2026
27 of 40 checks passed
@liamhuber liamhuber deleted the bump-executorlib branch March 3, 2026 15:04
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.

3 participants