fix grouping GT vs prediction by not assuming only pred have score

[bertsky]

This removes the assumption that only the prediction dataset contains score values.

[bertsky]

I've merged the current master to give the new CI a try. Unfortunately, the credentials problem persists...

The current Github workflow configuration still tries to build and push Docker images for pull requests. IMHO @i008 you need two different job definitions here, one for push to master (including all steps of the setup-build-publish job) and one for PRs (only Checkout and Build).

[i008]

If you work on the fork it will not work. Dockerhub credentials are added to the original repo and only accessible here. But I assume once you open a pr it will work properly?

…

On Wed, Mar 24, 2021, 12:28 AM Robert Sachunsky ***@***.***> wrote: I've merged the current master to give the new CI a try. Unfortunately, the credentials problem persists... The current Github workflow configuration still tries to *build and push Docker images for pull requests*. IMHO @i008 <https://github.com/i008> you need two different job definitions here, one for push to master (including all steps of the setup-build-publish job) and one for PRs (only Checkout and Build). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTZGEII5UO27LRWESABVXDTFEP2HANCNFSM4ZUCZXBQ> .

[i008]

I don't see much value in just building without pushing for manual testing.

…

On Wed, Mar 24, 2021, 12:32 AM Jakub Cieslik ***@***.***> wrote: If you work on the fork it will not work. Dockerhub credentials are added to the original repo and only accessible here. But I assume once you open a pr it will work properly? On Wed, Mar 24, 2021, 12:28 AM Robert Sachunsky ***@***.***> wrote: > I've merged the current master to give the new CI a try. Unfortunately, > the credentials problem persists... > > The current Github workflow configuration still tries to *build and push > Docker images for pull requests*. IMHO @i008 <https://github.com/i008> > you need two different job definitions here, one for push to master > (including all steps of the setup-build-publish job) and one for PRs > (only Checkout and Build). > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#14 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABTZGEII5UO27LRWESABVXDTFEP2HANCNFSM4ZUCZXBQ> > . >

[i008]

Ok it seems it's more complicated that this i guess using the github registry might be a better way. Will check.

[bertsky]

If you work on the fork it will not work. Dockerhub credentials are added to the original repo and only accessible here.
I don't see much value in just building without pushing for manual testing.

I disagree. Whether or not an image can be built at all is already a test on its own. Pushing certainly is not, and can (must) be reserved for master only.

As soon as someone comes up with more elaborate checks (unit tests or smoke tests for a demo on sample data), that could be integrated into both strains.

But I assume once you open a pr it will work properly?

No, because a PR is technically a branch on a fork (for which a ref/alias exists) – see above.

Ok it seems it's more complicated that this i guess using the github registry might be a better way. Will check.

I don't think it's complicated at all from here on. You've already covered all the bases, just differentiate the jobs for master and everything else (local branches or PR branches).

[i008]

I don't think it's complicated at all from here on. You've already covered all the bases, just differentiate the jobs for master and everything else (local branches or PR branches)

I ment its more complicated to have builds and pushing to a open registry, from a PR. not just commit to the repo .
Ofcourse building especially with testing adds value, but in such a tool what is more important i feel is to be able to easly test it manually, so for instance just downloading and image based on a PR/branch.

Anyhow i guess you are right i was not exactly sure what happens on a PR and wrongly assumed this might work:P

[i008]

@bertsky Please chceck the current workflow in master if this will work.

[bertsky]

Oh, now I understand. I am a bit surprised you prefer Docker for testing over a native venv, but in that case, I always try to keep an up-to-date bertsky/coco_explorer on Dockerhub with my latest PR adventures 😄

Please chceck the current workflow in master if this will work.

Will do...

[bertsky]

Please chceck the current workflow in master if this will work.

So partial success it is. PR still triggers the publish job (which fails), but also the build job.

Cannot open suggestions without an open PR for the CI, but I propose the following changes:

• during docker build, instead of unconditionally using both --tag i008/$IMAGE:$GITHUB_SHA and --tag i008/$IMAGE:latest, use only the former
• then in PublishLatest, before docker push, do a docker tag i008/$IMAGE:$GITHUB_SHA i008/$IMAGE:latest
• split the build.yml file into 2 separate files, one for the on: [push] with the publish job, and another for on: [pull_request] with the build job