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

Feature/concurrent build fixes #67 #71

Merged

Conversation

joshlk
Copy link
Collaborator

@joshlk joshlk commented Jul 4, 2022

Fixes #67

Copy link
Owner

@tbenthompson tbenthompson left a comment

Choose a reason for hiding this comment

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

This is awesome. Thank you so much! I have some comments through the PR about things that I'd like to do. I'm happy to resolve these if you're not interested or don't have time.

Also, I just added you as a collaborator on this project! That gives you permission to merge this in whenever you want - we'll have plenty of time to fix any small issues before another release.

Note:

  • The actions workflow missed this PR because currently the workflow trigger is on: [push]. I need to change this to on: [push, pull_request] so that PRs created based on branches inside forked repos are caught by the trigger.

cppimport/__init__.py Show resolved Hide resolved
cppimport/__init__.py Outdated Show resolved Hide resolved
cppimport/__init__.py Show resolved Hide resolved
@joshlk
Copy link
Collaborator Author

joshlk commented Jul 8, 2022

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
Copy link
Owner

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
Copy link
Owner

See the PR here: graphcore#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: #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
Copy link
Owner

Moved the two CI pipeline problems to their own issues: #74 #73

Run precommit, plus other small edits.
@joshlk
Copy link
Collaborator Author

joshlk commented Jul 8, 2022

Ok ready to merge 👍

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.

Race condition when multiple processes try to compile a module at once
2 participants