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

Fix issue #280 #281

Merged
merged 6 commits into from May 7, 2024
Merged

Fix issue #280 #281

merged 6 commits into from May 7, 2024

Conversation

lohedges
Copy link
Contributor

This PR closes #280.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@chryswoods

Note that I expect the CI to fail on macOS due to this issue.

@lohedges lohedges added bug Something isn't working exscientia Related to work with Exscientia labels Apr 29, 2024
@lohedges
Copy link
Contributor Author

Well, that was strange. The macOS build did indeed fail, but not for the reason that I was expecting. The error is:

 The reported errors are:
- Encountered problems while solving:
-   - nothing provides requested sire 2024.2.0.dev

The package does exist, so I don't know why it wasn't found. Will try re-running.

@lohedges
Copy link
Contributor Author

Same issue with a re-run. Really strange.

@lohedges
Copy link
Contributor Author

I see the issue, the runner is now osx-arm64. This must be the origin of the GROMACS issue too!

@lohedges lohedges temporarily deployed to biosimspace-build May 3, 2024 11:49 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build May 3, 2024 11:49 — with GitHub Actions Inactive
@xiki-tempula
Copy link
Contributor

Hi @lohedges Is this PR ready? Do you mind merge this, please? Thanks.

@lohedges
Copy link
Contributor Author

lohedges commented May 7, 2024

It's ready from a code perspective, but needs a few things first:

  • We need to update the sire CI to use the old x86 macOS runner, of move to macOS ARM64. (This is causing the macOS CI failure above, i.e. there's no matching macOS sire package to build against.)
  • Figure out the Windows CI failures that are causing the environment creation to fail. (Looks like a mamba issue on WIndows.) Looks like the environment failure issue has been fixed. The failure here was cause by one of the random IO errors that seems to happen when reading test files. Just re-running now.
  • Needs a review. I can ask @mb2055 to take a look if @chryswoods is too busy this week.

@lohedges
Copy link
Contributor Author

lohedges commented May 7, 2024

The Windows re-run passed. Everything should be good once the macOS ARM64 sire package is ready. If easier, we can always merge once reviewed and rebuild against the updated packages once available.

@xiki-tempula
Copy link
Contributor

Do you mind merge this now, please? Thank you.

@lohedges lohedges requested a review from mb2055 May 7, 2024 13:04
@lohedges
Copy link
Contributor Author

lohedges commented May 7, 2024

@mb2055: Are you able to review?

@xiki-tempula: I've already mentioned twice that we need a review for this. We have an internal process to follow with a limited team who are busy working on other things. I see that you have already manually applied on of the fixes here to your own fork, i.e. in this commit. I know your fork is based from our main, but is there a reason that you can't apply patches from fix PRs for things that are blocking you? (It seems that you do manually add certain fixes, but not others.)

@mb2055
Copy link
Contributor

mb2055 commented May 7, 2024

Sure thing, I'll go through it now

@xiki-tempula
Copy link
Contributor

Sorry, I was reading too fast and didn't noticed that it has not been reviewed. I thought that the only blocker is failed pipeline. I will manually apply the fix then.

@lohedges
Copy link
Contributor Author

lohedges commented May 7, 2024

Thanks. I'm off for the school run in a mo, but should be able to check back later depending on the level of carnage.

@lohedges
Copy link
Contributor Author

lohedges commented May 7, 2024

No problem. For fixes like this it is probably best (in future) that someone from your end reviews it, since you have a detailed understanding of the issue and can easily test if it works. I think you can add reviews (you have done before) but aren't available from the drop-down for some reason. (Presumably I need to adjust the permissions.)

@xiki-tempula
Copy link
Contributor

Thanks for mentioning it. I always thought that only the OBS people can be asked to review these PRs.

Copy link
Contributor

@mb2055 mb2055 left a comment

Choose a reason for hiding this comment

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

Everything working on my end 👍 sounds like everything is working for @xiki-tempula so it should be good to go

@lohedges lohedges merged commit 6d431c5 into devel May 7, 2024
4 of 5 checks passed
@lohedges lohedges deleted the fix_280 branch May 7, 2024 14:50
lohedges added a commit that referenced this pull request May 7, 2024
lohedges added a commit that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exscientia Related to work with Exscientia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] getFrame space check incorrecty uses self
3 participants