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

[DNM] Test osx arm64 #1913

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[DNM] Test osx arm64 #1913

wants to merge 2 commits into from

Conversation

mikemhenry
Copy link
Collaborator

See #1912

@mikemhenry
Copy link
Collaborator Author

I can add back in the other operating systems if we need to compare output, but I figured as a first pass we can see what sort of errors crop up.

Copy link

codecov bot commented Jul 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.71%. Comparing base (4ebc06f) to head (db9ab54).

Additional details and impacted files

@mikemhenry
Copy link
Collaborator Author

@j-wags Things are green, but lots of skips, know I know there are some tests for openeye and others for rdkit, but are any tests skiped via a heuristic like "antechaber failed so we will skip tests that use it" since that would give us misleading picture of what is going on. = 2302 passed, 9840 skipped, 7 xfailed, 2 xpassed, 282 warnings in 170.03s (0:02:50) =

@mikemhenry mikemhenry marked this pull request as draft July 26, 2024 20:21
@j-wags
Copy link
Member

j-wags commented Jul 26, 2024

are any tests skiped via a heuristic like "antechaber failed so we will skip tests that use it"

Not to the best of my knowledge. If RDK+AT are installed, the tests for those backends should be unconditionally run.

Thanks for your iteration on this. I'm out of the office today but can follow up on anything else Monday!

@mattwthompson
Copy link
Member

FWIW these fall under the umbrella of our ecosystem tests https://github.com/openforcefield/status/actions/runs/10136579213/job/28025563565

@mikemhenry
Copy link
Collaborator Author

So is there a test case missing that #1911 is exposing OR is it "we can't reproduce these errors on our test infrastructure?"

@mikemhenry
Copy link
Collaborator Author

Also thanks Matt, I didn't think to look there to see if you are testing on osx-arm64!

@mattwthompson
Copy link
Member

So is there a test case missing ... OR is it "we can't reproduce these errors on our test infrastructure?"

I strongly suspect it's the latter since reproducing it in isolation seems to be difficult.

FWIW, osx-arm64 tests have not been flaky in the months we've had them, at least not with respect to charge assignment. In principle, we want to wrap all of that investigation into a new unit tests, but that doesn't seem to be so simple for the steps needed to isolate what was reported there

@@ -27,7 +27,7 @@ jobs:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, macos-12]
os: [macos-14]
Copy link
Member

@mattwthompson mattwthompson Aug 2, 2024

Choose a reason for hiding this comment

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

Suggested change
os: [macos-14]
os: [ubuntu-latest, macos-12, macos-latest]

This changeset is strictly unnecessary but I think it'd be useful to have here instead of downstream

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I had intended to just test osx-arm64 -- but would you like to get osx-arm64 added into your CI matrix?

Copy link
Member

Choose a reason for hiding this comment

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

up to @j-wags in this project but it makes sense to test on all three IMHO

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