Skip to content

Check if cpu.max file is non-empty before reading#479

Open
mindflayer wants to merge 11 commits intojoblib:masterfrom
mindflayer:patch-1
Open

Check if cpu.max file is non-empty before reading#479
mindflayer wants to merge 11 commits intojoblib:masterfrom
mindflayer:patch-1

Conversation

@mindflayer
Copy link

Fix for #478

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

LGTM!

@tomMoral
Copy link
Contributor

It seems that this make some test fails: test_cpu_count_cgroup_limit.

@mindflayer
Copy link
Author

mindflayer commented Jan 28, 2026

It was a bit more complex than I firstly thought, and of course I had to try everything from an environment where Docker was setting everything up properly.
I added a way to skip the test when the environment is not configured correctly, not sure if that's what we want.
Then I added two more tests with edge cases. Let me know what you think!

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

Thanks a lot for investigating this.
It is indeed more complex than it looked 😅

This is just a few remarks from a quick look. The last test seems to not call the expected func?

The changes seem reasonable but I need more time to read them in detail, will try tomorrow.

mindflayer and others added 4 commits January 28, 2026 19:52
Co-authored-by: Thomas Moreau <thomas.moreau.2010@gmail.com>
@mindflayer
Copy link
Author

mindflayer commented Jan 28, 2026

The changes seem reasonable but I need more time to read them in detail, will try tomorrow.

Take your time, I had this error happening for ages. Today the hidden boyscout in me thought it was the time to stop rebooting the system just to test this project of mine. :)

@mindflayer
Copy link
Author

@tomMoral anything more I could do for helping out here?

Copy link
Contributor

@tomMoral tomMoral left a comment

Choose a reason for hiding this comment

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

Overall, LGTM, the change is reasonable and well tested.

Just a few more simplification for the code

@mindflayer mindflayer requested a review from tomMoral February 14, 2026 09:01
@mindflayer
Copy link
Author

I might be wrong, but the failure does not look like something related to this PR.

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.

2 participants