-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix CMake not adding some tests #104
Conversation
Now the CI is picking up all tests. |
Will do! |
Running here: https://github.com/openmm/NNPOps/actions/runs/5085521894 |
@RaulPPelaez good call with that 20min time out, it got stuck in a loop https://github.com/openmm/NNPOps/actions/runs/5085521894/jobs/9139106717 |
It does not seem stuck in a loop to me, it seems like it just takes too long. The waterbox test does take a long time in the CPU. See one of the CPU CI's https://github.com/openmm/NNPOps/actions/runs/5078935616/jobs/9124055531?pr=104 |
Also, no idea where this error comes from and why its only there in the GPU runner:
|
I've done this before with pytest + decorators. I'll first try bumping up the time-out first. I thought it was a loop since I wasn't looking closely and thought it kept running the same pytest tests, but now I see they are separate invocations. RE It does seem to be consistent. It shows up only for I will also look at cuda versions of the test since that will save time and money, but I want to get everything passing first. Thank you for your patience on this! |
looks like it is all the same file... maybe there is something weird with it? I'll see if I can reproduce on aws |
There is something weird with the file
|
Viewing the file, it does have junk in it: @RaulPPelaez @raimis |
What are you saying! Thank you, man!
I have no idea. |
There are other strange chars in that file:
Seems to me like that |
Ah i have seen this error before but it went away with a different MDTraj version so I didn’t investigate further |
I haven't really looked into how the tests are using this file, so perhaps the |
testing here: https://github.com/openmm/NNPOps/actions/runs/5469785425 |
Seems like the issue with the non-ascii files was fixed. All tests pass inyour GPU runner! |
So this should be good to merge in, any objections? |
Requested some reviewers |
One of the tests fails to install CUDA with "no space left on device", it happens sometimes and I do not think we can do anything about it. I will trigger a rerun |
Success! Please review again @peastman |
Looks good as far as I can tell. |
Sweet, I will re-run the GPU tests to make sure we are good and if they pass I will get this merged in! |
Testing here: https://github.com/openmm/NNPOps/actions/runs/5489633893 |
Solves #98