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

Re-run CI #203

Closed
wants to merge 7 commits into from
Closed

Re-run CI #203

wants to merge 7 commits into from

Conversation

mattwthompson
Copy link
Member

CI is failing on feature branches for reasons that are not clear to me. I'm re-running CI here to see if the failures are genuine and on the main branch.

@j-wags
Copy link
Member

j-wags commented Nov 15, 2022

Oh, wow, thanks for catching this.

It looks like 4 errors have started to happen:

  • The importlib thing which you caught here (and are fixing other places too)
  • A bunch errors around the pint 0.20 release which got resolved 15 days ago by the openff-units release
  • A test which has been flaky for a while openff/bespokefit/tests/executor/services/qcgenerator/test_worker.py::test_compute_torsion_drive where an optimization doesn't converge and raises geometric.errors.GeomOptNotConvergedError: Optimizer.optimizeGeometry() failed to converge.
  • Whatever this new thing is, in test_get_optimization which manifests as HTTPStatusError. I agree with you that it only seems to manifests on PRs from branches in the last few days.

So I'll keep looking into this last one, and I may work on that flaky convergence one as well.

@j-wags
Copy link
Member

j-wags commented Nov 15, 2022

Fastapi is a possible culprit here, though they have a really fast release pace so maybe it's nothing. https://anaconda.org/conda-forge/fastapi/files

@mattwthompson
Copy link
Member Author

mattwthompson commented Nov 15, 2022

Here's the most recent nightly CI run that passed: https://github.com/openforcefield/openff-bespokefit/actions/runs/3231103232/jobs/5290258911

@j-wags
Copy link
Member

j-wags commented Nov 15, 2022

Looks like that was it. Possible even rootier root issue is the fastapi conda package changing from requiring starlette 0.20 to 0.21. Rabbit hole begins here: tiangolo/fastapi#5646

@mattwthompson
Copy link
Member Author

That's exactly it; the currently-failing tests (👁️=false) are on Starlette 0.21 and the passing tests (👁️=true). I think that dependency update was intentional: https://github.com/tiangolo/fastapi/blob/4638b2c64e259b90bef6a44748e00e405825a111/docs/en/docs/release-notes.md#0870

FastAPI is known for being relatively unstable as a framework, which may or may not be fair since Django and Flask are a decade or so older and more hardened.

Let's see a different pin helps ...

@mattwthompson
Copy link
Member Author

^ above failures vs. passing tests is because I forgot to update both environments, not necessarily because of some mystical relationship between OpenEye and Starlette

@mattwthompson
Copy link
Member Author

If this pin works, my current plan is:

I can also push the pin into the conda builds if you think that's worthwhile

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #203 (19c090a) into main (5663b52) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@j-wags
Copy link
Member

j-wags commented Nov 15, 2022

That plan sounds good to me, and I agree we should pin the conda builds as well!

I'd like to gain a better understanding of where the rift in our fitting stack is going to open up - There's some range of packages that work with OFFTK 0.10, and another that works with OFFTK 0.11.

Also I'm a little bothered by the sporadic GeomeTRIC convergence errors, but I'll open a separate issue for that.

@mattwthompson
Copy link
Member Author

Merging these fixes as part of #202 since these environments are pretty slow to solve (one took 50 minutes)

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.

None yet

2 participants