InvokeAI icon indicating copy to clipboard operation
InvokeAI copied to clipboard

Do not build Docker images on PR

Open ebr opened this issue 3 years ago • 5 comments

A few CI optimizations:

  • cloud docker images will no longer build on PRs (requested by @mauwii )
  • instead, build cloud docker images on all tags to make a variety of image versions available (not only latest)
  • cancel redundant workflows; this will free up CI runners and should greatly speed things up
  • remove conda test workflows as we've deprecated conda installs in 2.2.4

ebr avatar Dec 11 '22 19:12 ebr

@mauwii Could you give this a code-owner review?

lstein avatar Dec 12 '22 04:12 lstein

I am a bit confused, shouldn't the removement of Conda Workflows be it's own PR? I asked this 3 times yesterday in discord and nobody responded to this, now it is part of a PR Regarding the Docker Cloud Image, this feels strange ... Also the Docs still refer to the conda Installation method, so I am not sure if it is rly deprecated: image

What I rly luv and did not know yet --> CONCURENCY - I ALREADY LUV IT!!!! With this, imho, we could still build the docker image on PRs if you want them to. But yesterday we had soooo many actions in the queue, which is why I asked if we could disable PR Builds of the container. But thx to concurency, this is maybe not rly required anymore. Will update the Test Actions later to also include it!

Edit: Ah - you also already do so in this PR, well, thx for that I guess 🙈

mauwii avatar Dec 12 '22 09:12 mauwii

so @lstein: Do we want to get rid of the conda test action, and if yes, should it be Part of this PR?

I would recommend, if we refer to conda in the docs, we also make at least sure it is installable.

mauwii avatar Dec 12 '22 09:12 mauwii

I also think concurrency should not be enabled for the container image workflow, when it is only build for main branch pushes at all. This way you would have the ability to choose whichever commit suits you if you do not want to use latest.

But in #1950 I updated the build-container.yml workflow which would enable us to use only one workflow to build/push the container images

mauwii avatar Dec 12 '22 16:12 mauwii

so @lstein: Do we want to get rid of the conda test action, and if yes, should it be Part of this PR?

I would recommend, if we refer to conda in the docs, we also make at least sure it is installable.

So far I've updated the documentation to indicate that the conda install method is deprecated and that support will be dropped "in the future." This is in one of the doc PRs I submitted yesterday, not sure if it is merged yet. I would very much like to get rid of the conda CIs, but we can't do it until we officially drop support.

I'm not sure about the timing, but I think at least another week in order to avoid alienating developers who have already struggled through the changes in the init file and runtime directory.

lstein avatar Dec 13 '22 12:12 lstein

I just created a new PR to add the concurrency in the two test workflows, and I set them up in a way that it only will cancel actions in PRs if a new commit comes in, but does not cancel running workflows in a branch (f.E. main), so that it is easier in the end to see where a breaking change was introduced if any slips through (when it would cancel runs in the main branch, they get the red X which will not only mess up the readme (CI Status on main), but also make it impossible to find out when things started to fail).

mauwii avatar Dec 13 '22 18:12 mauwii

@mauwii I'm a little unclear on why you needed to create a new PR that straight up duplicates my changes, and then merge it without review... but i guess as long as the code is committed, that's fine by me.

ebr avatar Dec 13 '22 22:12 ebr

@mauwii I'm a little unclear on why you needed to create a new PR that straight up duplicates my changes, and then merge it without review... but i guess as long as the code is committed, that's fine by me.

I did not want to make changes to your PR and since I waited already 2 days for answers, I went ahead and created a separate one. Also this PR is called "Do not build Docker images on PR" which obviously has nothing to do with the two test workflows.

And it was merged without review, but I spoke to @blessedcoolant and @lstein beforehand in discord, so I did not just merge what just me thought is good for us all...

mauwii avatar Dec 13 '22 22:12 mauwii

can be closed since cloud-image is not working multiarch and the default Dockerimage already has cloud ability. Will open a new PR to remove uneeded Files and update the docs.

mauwii avatar Dec 14 '22 06:12 mauwii