search

tbenthompson / cppimport

Import C++ files directly from Python!
MIT License
1.18k stars 67 forks source link

Feature/concurrent build fixes #67 #71

Closed joshlk closed 2 years ago

joshlk commented 2 years ago

Fixes #67

joshlk commented 2 years ago

I tried modifying the CI yaml to include pull requests and it says:

1 workflow awaiting approval. First-time contributors need a maintainer to approve running workflows.

If you approve it - it should work

tbenthompson commented 2 years ago

Just a heads up that I'm going to fix the CI failures and make a few edits. Didn't want to duplicate the work with you. =)

tbenthompson commented 2 years ago

See the PR here: https://github.com/graphcore/cppimport-fork/pull/2

Copied the comment there:

I opened this PR here so that it'll still be your PR that gets merged into cppimport.

pre-commit: black, isort, flake8 and corresponding small edits little comment about try_load raise an exception if a concurrent build is detected and force_rebuild is True changed "hook_test.cpp" to "tests/hook_test.cpp". The CI runs the tests from the project root so paths in the tests need to have that form. See the CI success here: https://github.com/tbenthompson/cppimport/pull/72 - I'll close that PR after yours is merged. I'm not worried about the codecov decrease. =/

The only remaining issue here is that one of the tests on Mac/Python 3.10 failed once: https://github.com/tbenthompson/cppimport/runs/7253199285?check_suite_focus=true

It's a stochastic failure and I can't replicate it. I can't quite explain this failure. My first guess was that it's happening when the built extension hasn't quite been fully written out. But that doesn't make sense because the file lock won't be released until after the extension has been fully written. So, given my lack of understanding I did two things:

  1. I set buffering=0 on the checksum trailer save so that it's saved immediately.
  2. I added an OSError check on the checksum trailer load so that we'll just rebuild if something weird happens. This seems okay given the failure is rare.

The "Build and publish" CI step is failing because I set it up a bit wrong, but that shouldn't be your concern.

I think this is 100% ready to merge. Let's do it!

  1. You merge my PR into your branch
  2. Then, we can merge this PR.
tbenthompson commented 2 years ago

Moved the two CI pipeline problems to their own issues: https://github.com/tbenthompson/cppimport/issues/74 https://github.com/tbenthompson/cppimport/issues/73

joshlk commented 2 years ago

Ok ready to merge 👍