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

Disable multi-core pyiron table test for mac os x #1383

Closed
wants to merge 1 commit into from

Conversation

jan-janssen
Copy link
Member

@jan-janssen jan-janssen commented Mar 17, 2024

This fixes the broken Mac OS X tests.

@jan-janssen
Copy link
Member Author

jan-janssen commented Mar 17, 2024

While I hoped introducing the with-statement in #1384 would fix the issue - it does not. So I temporarily disable the corresponding test.

@jan-janssen jan-janssen deleted the mac_disable_test branch March 17, 2024 16:44
@jan-janssen jan-janssen restored the mac_disable_test branch March 18, 2024 21:51
@jan-janssen jan-janssen reopened this Mar 18, 2024
@jan-janssen jan-janssen requested a review from liamhuber March 18, 2024 22:07
Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

This fixes the broken Mac OS X tests.

It hides them, it doesn't fix them.

If we can check for the system and pass in the tests, we can check for the system and fail cleanly with an informative error message (and then test for that case) in the codebase itself. This also doesn't truly "fix" the problem either, but at least it puts the code in an understood state.

@jan-janssen
Copy link
Member Author

It hides them, it doesn't fix them.

Yes, because I think a partially working test on Mac OS X is more valuable than no test on Mac OS X and partially in this case means, all but one.

@liamhuber
Copy link
Member

It hides them, it doesn't fix them.

Yes, because I think a partially working test on Mac OS X is more valuable than no test on Mac OS X and partially in this case means, all but one.

I'm not advocating that we should bail on OSX here, I'm saying instead of skipping the test and leaving users to discover some weird hang state lets fix it so it fails clean! Then the tests will be passing because they return a known state, that is, failure on OSX for this particular operation.

In the long run, of course, we want the parallel operation working on OSX too, but since nobody seems to have any clue how to accomplish that, let the codebase reflect it!

@jan-janssen
Copy link
Member Author

In the long run, of course, we want the parallel operation working on OSX too, but since nobody seems to have any clue how to accomplish that, let the codebase reflect it!

It is just a single feature that is not working on OSX. This pull request localises the issues. So the test for this feature is skipped by marking the corresponding test. For me this is a totally different statement, than saying pyiron_base is not working on Mac OS X. Consequently, I am still I favour of merging this pull request, especially as nobody else had the time to look into this issue in the last three weeks, so I do not see anybody having the time to fix it soon.

@liamhuber
Copy link
Member

Ok, I try one more time: there is a feature that we know will break for users under certain conditions, but it won't fail cleanly or in an informative way. We know this because the CI whines to us with a red flag. You have nailed down exactly the part of the code that is failing and isolated it with a flag.

Now use that flag to give the user an informative error (and catch that error using the same flag in the tests) instead of using it to tell the CI to just shut up.

I don't understand why this is even a fight. Of course we both want the tests passing. You've already done all the necessary sleuthing and almost all the necessary leg work for catching the issue cleanly for users. Why just leave the code with a dirty break??

@jan-janssen
Copy link
Member Author

Locally everything works fine - it is a Github CI specific issue. I now added the workaround in #1390 and opened an issue with the Github Images team actions/runner-images#9534 .

@jan-janssen jan-janssen deleted the mac_disable_test branch March 19, 2024 05:34
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.

OSX 12 tests are sometime timing out
2 participants