Major refactor of helper functions #167

chrisvanrun · 2024-08-27T09:58:25Z

This PR refactors existing helper functions (i.e. Client.update_archive_item) and adds some new ones that make sense.

These are the helper functions now:

update_archive_item
add_cases_to_archive (new)
update_display_set (new)
add_cases_to_reader_study
run_external_job
upload_cases (untouched)

Proto models

The core of this PR is the 'gcapi/models_proto.py'.

This PR introduces proto-models. These are models of things that are yet to be. Their main function is to separate the creation/saving logic from the validation logic. This allows for quick unit tests to cover corner cases in validation, while simultaneously allowing to validate all values before anything hits the platform.

These 'preflight' validations were, partially, already in place for add_cases_to_reader_study but are now ubiquitous for all helper functions.

While refactoring I ran into several issues concerning the adaptation of models, which I fixed as I went. With deadline pressure and random fixes, flaky tests, et cetera this is now a messy and large change set. Sorry!

Below are some descriptions of additional fixes that are included in the PR.

Both `str`, and [`str`] are allowed

Sourcing a CIV from one file no longer requires a one-lengthed list, the helper functions automagically handle both. This also applies to pathlib.Path.

`.details` API now accepts an `api_url` keyword argument

Since we are casting to models it made more sense to be able to call the .details with api_url than to manually figure out which gcapi.models one is to use.

This replaces:

 gcapi.models.HyperLinkedImage(**client(url=api_url))

With:

client.images.details(api_url=api_url)

`.create()`/`.update()`/`.partial_update()` -> models

API calls that support .create(..), .update(..), and .partial_update(..) now return gcapi.models instead of dicts.

Using existing images

Related to the actual pitch:

https://github.com/DIAGNijmegen/rse-roadmap/issues/305

One can add an existing image as follows (pulled from a test):

 # First create an image component interface
added_display_sets = client.add_cases_to_reader_study(
    reader_study="reader-study",
    display_sets=[
        {
            "ct-image": [TESTDATA / "image10x10x101.mha"],
        }
    ],
)

display_set_pk = added_display_sets[0]

# ... wait for import to finish

display_set = c.reader_studies.display_sets.detail(pk=display_set_pk)

# Re-use both as a direct image and a CIV
display = display_set.values[0]
image = client.images.detail(api_url=display.image)

added_display_sets = await client.add_cases_to_reader_study(
    reader_study="reader-study",
    display_sets=[
        {
            "ct-image": image,
        },
        {
            "ct-image": civ,
        },
    ],
)

Some typing stuff

Closes: #163

Some methods do not seem to return models #163

Usage of `JSON` for `"File"` super_kind interfaces.

Closes: #153

Uploads to archive items fail when CIV is file type and JSON sent #153

It is now possible to provide:

a file path to a super-kind:"Value" interface
a dict (or other JSON-like python object) to a super-kind:"File" interface

Note that for the "File" one, this cannot be done for an interface with kind "String". It would be far too easy to have a non-existing file path as content for a String interface. All other interfaces should error out on the platform and run into schemas there.

…e-super_kind interfaces

This reverts commit d50d251.

chrisvanrun · 2024-08-27T10:22:58Z

tests/integration_tests.py

    )

    # retrieve existing archive item pk
    items = get_archive_items(c, archive.pk, len(old_items_list))

    old_civ_count = len(items[-1].values)

-    with pytest.raises(ValueError) as e:


Covered by unittests

chrisvanrun · 2024-08-27T10:24:00Z

tests/integration_tests.py

-                },
-                "predictions-csv-file": [
-                    Path(__file__).parent / "testdata" / "test.csv"
+CIV_SET_PARAMS = (


Defining this 4x seemed a bit verbose.

chrisvanrun · 2024-08-27T10:24:36Z

tests/integration_tests.py



-def test_add_cases_to_reader_study_multiple_files(local_grand_challenge):


Covered by unittests

chrisvanrun · 2024-08-27T13:19:15Z

Ugh. I was afraid of this. There are some race conditions between a RawImageUpload being processed, a CIV being created and the image being assigned. Resulting in flaky tests. Fixing!

chrisvanrun · 2024-08-27T18:55:54Z

Fingers crossed... locally everything ran butter smooth. It's the CI restraints that show the race conditions.

chrisvanrun · 2024-08-27T18:57:48Z

DeprecationWarnings, fixing...

/home/runner/work/rse-gcapi/rse-gcapi/gcapi/client.py:510: DeprecationWarning: Using ["api_url"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["api_url"] with .api_url
      uploads=[u["api_url"] for u in uploads], **kwargs
  tests/integration_tests.py::test_upload_cases_to_archive[files1-None]
  tests/integration_tests.py::test_upload_cases_to_archive[files0-generic-overlay]
  tests/integration_tests.py::test_upload_cases_to_archive[files2-generic-overlay]
  tests/integration_tests.py::test_upload_cases_to_archive[files3-None]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:150: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      us = get_upload_session(c, us["pk"])
  tests/integration_tests.py::test_upload_cases_to_archive[files1-None]
  tests/integration_tests.py::test_upload_cases_to_archive[files0-generic-overlay]
  tests/integration_tests.py::test_upload_cases_to_archive[files2-generic-overlay]
  tests/integration_tests.py::test_upload_cases_to_archive[files3-None]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:178: DeprecationWarning: Using ["files"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["files"] with .files
      response = c(url=image["files"][0]["file"], follow_redirects=True)
  tests/integration_tests.py::test_upload_cases_to_archive[files1-None]
  tests/integration_tests.py::test_upload_cases_to_archive[files0-generic-overlay]
  tests/integration_tests.py::test_upload_cases_to_archive[files2-generic-overlay]
  tests/integration_tests.py::test_upload_cases_to_archive[files3-None]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:178: DeprecationWarning: Using ["file"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["file"] with .file
      response = c(url=image["files"][0]["file"], follow_redirects=True)
  tests/integration_tests.py::test_download_cases[files0]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:283: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      us = get_upload_session(c, us["pk"])
  tests/integration_tests.py::test_create_job_with_upload[test-algorithm-evaluation-image-1-generic-medical-image-files0]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:339: DeprecationWarning: Using ["status"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["status"] with .status
      assert job["status"] == "Queued"
  tests/integration_tests.py::test_create_job_with_upload[test-algorithm-evaluation-image-1-generic-medical-image-files0]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:340: DeprecationWarning: Using ["inputs"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["inputs"] with .inputs
      assert len(job["inputs"]) == 1
  tests/integration_tests.py::test_create_job_with_upload[test-algorithm-evaluation-image-1-generic-medical-image-files0]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:341: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      job = c.algorithm_jobs.detail(job["pk"])
  tests/integration_tests.py::test_upload_cases_to_archive_item_with_new_interface
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:258: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      us = get_upload_session(c, us["pk"])
  tests/integration_tests.py::test_upload_cases_to_archive_item_with_existing_interface
    /home/runner/work/rse-gcapi/rse-gcapi/tests/integration_tests.py:227: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      us = get_upload_session(c, us["pk"])
  tests/async_integration_tests.py::test_download_cases[files0]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/async_integration_tests.py:328: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      us = await get_upload_session(c, us["pk"])
  tests/async_integration_tests.py::test_upload_cases_to_archive[files1-None]
  tests/async_integration_tests.py::test_upload_cases_to_archive[files2-generic-overlay]
  tests/async_integration_tests.py::test_upload_cases_to_archive[files3-None]
  tests/async_integration_tests.py::test_upload_cases_to_archive[files0-generic-overlay]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/async_integration_tests.py:170: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      us = await get_upload_session(c, us["pk"])
  tests/async_integration_tests.py::test_upload_cases_to_archive_item_with_existing_interface
    /home/runner/work/rse-gcapi/rse-gcapi/tests/async_integration_tests.py:258: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      us = await get_upload_session(c, us["pk"])
  tests/async_integration_tests.py::test_create_job_with_upload[test-algorithm-evaluation-image-1-generic-medical-image-files0]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/async_integration_tests.py:381: DeprecationWarning: Using ["status"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["status"] with .status
      assert job["status"] == "Queued"
  tests/async_integration_tests.py::test_create_job_with_upload[test-algorithm-evaluation-image-1-generic-medical-image-files0]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/async_integration_tests.py:382: DeprecationWarning: Using ["inputs"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["inputs"] with .inputs
      assert len(job["inputs"]) == 1
  tests/async_integration_tests.py::test_create_job_with_upload[test-algorithm-evaluation-image-1-generic-medical-image-files0]
    /home/runner/work/rse-gcapi/rse-gcapi/tests/async_integration_tests.py:384: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk
      job = await c.algorithm_jobs.detail(job["pk"])
  tests/async_integration_tests.py::test_upload_cases_to_archive_item_with_new_interface
    /home/runner/work/rse-gcapi/rse-gcapi/tests/async_integration_tests.py:301: DeprecationWarning: Using ["pk"] for getting attributes is deprecated and will be removed in the next release. Suggestion: Replace ["pk"] with .pk

.. Done!

chrisvanrun · 2024-08-27T20:02:31Z

How can it be that something that is referenced via a hyperlink from the API (e.g. via https://gc.localhost/api/v1/cases/images/8e3e90d5-7622-4574-84fe-82bbfadcd2b7/) SOMETIMES, but then consistently, gives a 404?

"Sometimes" :- because it is flaky.
"Consistently" :- because it is wrapped in a recurse_call which retries it every 500ms for 30 seconds.

chrisvanrun · 2024-08-28T07:15:07Z

Ok, possible fix: not recursing in tests when ObjectNotFound is thrown (instead of a HTTPStatusError)

chrisvanrun added 25 commits August 20, 2024 11:43

Base implementation of CIVSource and validations

3dfd816

Minor refactor of upload sources and allow for values as input to fil…

73e2f76

…e-super_kind interfaces

Initial version of add_civ_sets

b8377cd

Refactor proto civ

459fda4

Minor improvements

a60beaa

Add model casting to ModifiableMixin

22731a9

Disable https checks (should revert)

d50d251

Correctly implement generators in protoCIV

2824521

Simplify interface cache

092c482

Fix upload source tests

b39de18

Rename upload sources

f384e9b

Apply fixes for tests

a4eea12

Extend add_cases_reader_study tests

a9ceb3b

Add ProtoJob

7284545

Convert update_archive_item, minor refactors

069b73c

Add api_url detail retrieval and test for HyperlinkedImage

dcad7e0

Add add_cases_to_archive

baf2eae

DRY civ_set tests

1022c00

Add update_display_set

a636667

Update docstring

89c5ff7

Revert "Disable https checks (should revert)"

7a92f50

This reverts commit d50d251.

Revert local api URL change

b9c4932

Test for HyperlinkedComponentInterfaceValueFactory

48abf62

Unflaky an order based integration test

97d74c6

Minor typing

ffbb9a9

chrisvanrun commented Aug 27, 2024

View reviewed changes

chrisvanrun added 2 commits August 27, 2024 13:10

Check for pk + api_url + params

aa90608

Fix non-model get_file usage

4613179

chrisvanrun added 6 commits August 27, 2024 13:47

Remove '# type: ignore'

b6c7266

Fix incorrect kwargs

e7dae9e

Make test more robust: do not use iterator when detail suffices.

4526306

REALLY remove the # type: ignore

d6bd5a3

Minor syntax

d4f923c

Fix copy+pasta addition of awaits

3a92a4b

Add race-safe check for complete civ_set

a2b23e4

Fix deprecation warnings in tests

d4278cb

Also recurse when ObjectNotFound is thrown.

1a458cb

chrisvanrun marked this pull request as ready for review August 28, 2024 07:21

chrisvanrun requested a review from jmsmkn August 28, 2024 09:47

chrisvanrun changed the title ~~[WIP] Major refactor of helper functions~~ Major refactor of helper functions Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Major refactor of helper functions #167

Major refactor of helper functions #167

chrisvanrun commented Aug 27, 2024 •

edited

Loading

chrisvanrun Aug 27, 2024 •

edited

Loading

chrisvanrun Aug 27, 2024

chrisvanrun Aug 27, 2024

chrisvanrun commented Aug 27, 2024

chrisvanrun commented Aug 27, 2024

chrisvanrun commented Aug 27, 2024 •

edited

Loading

chrisvanrun commented Aug 27, 2024 •

edited

Loading

chrisvanrun commented Aug 28, 2024



		def test_add_cases_to_reader_study_multiple_files(local_grand_challenge):

Major refactor of helper functions #167

Are you sure you want to change the base?

Major refactor of helper functions #167

Conversation

chrisvanrun commented Aug 27, 2024 • edited Loading

Proto models

Both str, and [str] are allowed

.details API now accepts an api_url keyword argument

.create()/.update()/.partial_update() -> models

Using existing images

Some typing stuff

Usage of JSON for "File" super_kind interfaces.

chrisvanrun Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

chrisvanrun Aug 27, 2024

Choose a reason for hiding this comment

chrisvanrun Aug 27, 2024

Choose a reason for hiding this comment

chrisvanrun commented Aug 27, 2024

chrisvanrun commented Aug 27, 2024

chrisvanrun commented Aug 27, 2024 • edited Loading

chrisvanrun commented Aug 27, 2024 • edited Loading

chrisvanrun commented Aug 28, 2024

chrisvanrun commented Aug 27, 2024 •

edited

Loading

Both `str`, and [`str`] are allowed

`.details` API now accepts an `api_url` keyword argument

`.create()`/`.update()`/`.partial_update()` -> models

Usage of `JSON` for `"File"` super_kind interfaces.

chrisvanrun Aug 27, 2024 •

edited

Loading

chrisvanrun commented Aug 27, 2024 •

edited

Loading

chrisvanrun commented Aug 27, 2024 •

edited

Loading