DIAGNijmegen / rse-gcapi

Python client for the grand-challenge.org API
Apache License 2.0
2 stars 3 forks source link

Add back removed job and archive item tests #161

Open jmsmkn opened 1 month ago

jmsmkn commented 1 month ago

In #154 two tests were removed as the functionality was removed. It is planned to add this functionality back in https://github.com/DIAGNijmegen/rse-roadmap/issues/335, so the tests also need to be added here. See #155.

amickan commented 3 weeks ago

The functionality tested with test_update_interface_kind_of_archive_item_image_civ does not exist on GC anymore. It was removed in https://github.com/comic/grand-challenge.org/pull/3156

It's not straight-forward to add this back now that the CIV creation is standardized everywhere and if this should only be possible for archive items. If we do want this, it will need to be done in a seperate PR from the main refactoring PR that I'm currently working on.

amickan commented 3 weeks ago

Having looked at this some more, I wonder if we really need this functionality? Users can just delete archive items and display sets and recreate them with the correct interface, if need be. So from a user perspective, updating the interface kind is no longer necessary. Of course, if they go this route, we end up with duplicate data, but how often does someone do this at the moment anyway? Implementing the option to update the interface is not straight-forward at all and introduces a lot of complexity to the CIV creation & validation code. I vote to drop this.

jmsmkn commented 3 weeks ago

I don't think they can do that as if they remove the image from the display set they lose permissions unless it is held in another display set.

jmsmkn commented 3 weeks ago

And this happens all the time, especially when people upload images as the wrong type, or want to use the image with another algorithm when the interface is different.

amickan commented 3 weeks ago

I don't think they can do that as if they remove the image from the display set they lose permissions unless it is held in another display set.

Ok, so we want this also for display sets? Previously this was only implemented for archive items, as far as I know. So then the requirement is that this works for display sets and archive items and should it also work for all interface types of only for images?

amickan commented 3 weeks ago

or want to use the image with another algorithm when the interface is different.

This is a different issue. They can select existing images when creating an algorithm job, so that's already possible.

amickan commented 3 weeks ago

should it also work for all interface types of only for images

Actually this is only feasible for images. For values and json files changing the interface might invalidate the value/file, so for those it doesn't make sense.

jmsmkn commented 3 weeks ago

Should work the same everywhere, so for jobs, archive items or display sets. Only doing this for images is fine, those data are much larger and something we do not want to duplicate or make the user wait a long time to re-import.

amickan commented 3 weeks ago

For jobs updating the interface does not make sense. A job can only be created with the interfaces that are defined on the algorithm at job creation time. We don't update job inputs later on. Or am I missing something?

jmsmkn commented 3 weeks ago

That's true, thanks.

chrisvanrun commented 2 weeks ago

AFAIK test_create_job_with_upload is already in the main branch atm.

chrisvanrun commented 2 weeks ago

Note: with the major refactor of helper functions, using an existing image or CIV for the create_job_with_upload comes out of the box.