-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Conda build #38796
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
Conda build #38796
Conversation
💊 CI failures summary and remediationsAs of commit e7a6aa3 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 48 times. |
@malfet I assume at some point I will need to update the |
8f761f7
to
37ce0fe
Compare
Rebased off master to clear the merge conflicts. @malfet I still don't understand how to get the proper hash to put into the ignore stanza, and it seems the actual docker image build is not being triggered. |
ping @malfet |
@seemethere is there a way to move this forward? I am stuck trying to generate a new Docker image for the CI run |
This PR is still stuck waiting for instructions on how to generate a new CI docker image. |
@ezyang is there something I can do to move this forward? |
Hi @mattip, a lot of us are busy right now because it is performance cycle at FB. I don't have too much context on this diff, but if I understand correctly, on master, it's no longer necessary to manually delete the trigger strings, as this diff from @seemethere has landed: #40194. This means that the instructions at https://github.com/pytorch/pytorch/wiki/Docker-image-build-on-CircleCI are out of date. All you have to do now is submit a PR with the new Docker changes, and CI will automatically build the new images and test against them, and if all is green and this PR is approved I can land it. |
I tried to add a |
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
I read over the CircleCI yaml, and it looks like I was mistaken and it's still necessary to delete the triggers, sorry :( I'm going to try pushing the necessary update to trigger the build. Also, it looks like you don't have the ECR repo setup so I went ahead and did that for you. For future reference, try to avoid force pushing to the branch, it destroys history on GitHub which means I can't look at old builds and see if you previously had things setup correctly. |
Sorry. So the preferred workflow is to merge master into the branch? |
Yes, just merge in master. When we land the diff we'll squash all history so the merges won't matter in the long run. |
This reverts commit 4c7188d.
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
It seems to work, the test failures are the same as the other builds. |
I updated the tag from |
The lint quickcheck of |
8bdba785b1eac4d297d5f5930f979518012a56e0 looks wrong, there is no pytorch-linux-bionic-py3.7-conda build. You want d368bea1-af47-48fd-a3b7-c41d1e7b4f1a |
This reverts commit 585bd42.
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
The When I look at some of the other successful docker builds, the "Check if image should be built" step also looks for |
There's a bug; the docker jobs are checking for the wrong hash. The bug was exacerbated by the fact that streamlined docker build workflow was only partially landed (and part of it was reverted.) I think we will need to work around the bug for now, probably by deleting the "should build image" test. cc @seemethere I don't have time to fix this today due to PSC but will get back on it tomorrow. |
OK, turns out I was wrong.
This is wrong: the image does exist, as per:
It isn't showing up in the index, probably because the index hasn't picked up the new ECR repo. So actually you were all fine. The real reason why quickcheck lint was failing, was because there was an update with master, and the quickcheck is computed after merging with master (kind of icky, but you would have indeed broken quickcheck if you had merged as is. Would have been nice if it mentioned this though.) I'm going to revert back to your diffs and try to land. |
Thanks. I still think think it is fragile to tag the images with the |
I think in the half-constructed state we are in today, you basically can't use workflow ids, for the reasons you posted above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Woot! 🎉 Thanks Matti! 😄 |
This reverts commit 9c7ca89. Signed-off-by: Eli Uriegas <eliuriegas@fb.com>
This PR was reverted, with reason not documented and no further follow-up as far as I can tell. And right now there's again no CI job with conda compilers, and the build seems to be broken for at least some people (gh-47717). @seemethere do you remember what happened here? Can we re-introduce a conda compiler CI job? |
closes gh-37584.
I think I need to do more to generate an image, but theDropped updating.circleci/README.md
is vague in the details. The first commit reflows and updates that document a bit, I will continue to update it as the PR progresses :).circleci/README.md
, will do that in a separate PR once this is merged.