Server tests #502

hasan7n · 2023-11-19T19:16:47Z

This PR adds tests for all server endpoints, focusing on permissions / disallowed actions.

TODO: update this. EDIT: done

closes #509

github-actions · 2023-11-19T19:16:59Z

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

These endpoints are not used, and don't seem to help in our workflow. No need to keep maintaining them

PUT now is only permitted to the admin users We need to define what can be edited and how to sync with Auth0 before exposing this to the end users

Users were able to edit results dict and associated entities

users have to include hashes for URLs on mlcube creation but they were able to set it blank later

make dataset association private, return necessary fields to help the client know the latest approval status

this field was updated for any request containing approval_status

aristizabal95 · 2023-11-21T15:49:35Z

cli/medperf/comms/rest.py

@@ -183,8 +183,10 @@ def get_benchmark_models(self, benchmark_uid: int) -> List[int]:
        Returns:
            list[int]: List of model UIDS
        """
-        models = self.__get_list(f"{self.server_url}/benchmarks/{benchmark_uid}/models")
-        model_uids = [model["id"] for model in models]
+        # TODO: filter to find approved ones only if we decide to have


Should we handle filtering within the comms layer? Or within the entity layer? I think we've done filtering on the entity layer so far, because we also need to apply filtering to local entities

It's tricky for associations since we don't have an association Entity on the client yet.

Filtering associations requires two steps:

first, finding the latest association of each mlcube. Ideally this should live in an association Entity, but for now I think this should live in the comms layer, since the client doesn't actually care about the history of associations. Also, this logic is required for all places in the client code where this comms function is called.

Then, ordinary filtering by approval status. Currently this filtering is required in two places:
a. entity.list command: which already applies filtering in the commands layer.
b. benchmark.get_models_uids: which by definition should return approved associations model IDs. We can apply filtering inside this function.

I think this will become cleaner if we decide to create an association Entity in our code base.

- added is_valid and user_metadata fields - PUT and DELETE now are only for admin

This reverts commit 152862a. and deactivates the endpoints

doing this requires its three mlcubes to be operation

aristizabal95

Hey Hasan! Just left a few comments/questions for the future. I skimmed over the server tests and implementation. Everything looks great and the tests look very well organized. Amazing amazing work as always Hasan!

aristizabal95 · 2023-11-27T21:58:02Z

cli/medperf/tests/entities/test_benchmark.py

-        assert benchmark.reference_model_mlcube in benchmark.models
-
-    def test_benchmark_includes_additional_models_in_models(self, setup):
+    def test_benchmark_get_models_works_as_expected(self, setup, expected_models):


Note for the future: I'm not a big fan of the "works_as_expected" naming convention. We should find a way of shortly describing what's that expected behavior we're testing. No need to make any change now, but I think I've seen it a few times already and it's not as descriptive as a test should be

that's a very valid point

aristizabal95 · 2023-11-27T21:59:40Z

server/README.md

@@ -1,3 +1,64 @@
 # Server

 Documentation TBD
+
+## Writing Tests


I love that you added this! Thanks for the documentation Hasan!

aristizabal95 · 2023-11-27T22:00:35Z

server/benchmark/models.py

@@ -20,7 +20,7 @@ class Benchmark(models.Model):
    description = models.CharField(max_length=100, blank=True)
    docs_url = models.CharField(max_length=100, blank=True)
    owner = models.ForeignKey(User, on_delete=models.PROTECT)
-    demo_dataset_tarball_url = models.CharField(max_length=256, blank=True)
+    demo_dataset_tarball_url = models.CharField(max_length=256)


I remember we talked about this before, but can you remind me how we're getting past situations where demo datasets can't be shared directly with a link?

currently we support using synapse storage (e..g, Verena used a private demo dataset)
we should eventually support things like dropbox or google drive

hasan7n requested a review from a team as a code owner November 19, 2023 19:16

hasan7n had a problem deploying to testing-external-code November 19, 2023 19:16 — with GitHub Actions Failure

hasan7n added 14 commits November 20, 2023 10:51

write all tests

849b52f

change: remove unused endpoints

152862a

These endpoints are not used, and don't seem to help in our workflow. No need to keep maintaining them

change: fix PUT user permission

a9abb6d

PUT now is only permitted to the admin users We need to define what can be edited and how to sync with Auth0 before exposing this to the end users

change: fix PUT result behaviour

21c8cef

Users were able to edit results dict and associated entities

change: remove URLs from cube uniqueness criteria

85887ba

change: apply mlcube rules on both PUT and POST

fae91c0

users have to include hashes for URLs on mlcube creation but they were able to set it blank later

improve readability

76b05ab

change: modify how associations list works

14ecc83

make dataset association private, return necessary fields to help the client know the latest approval status

change: fix default approval status

0195b36

change: fix approved_at update

f4ff5df

this field was updated for any request containing approval_status

add notes and TODOs

7695dab

update workflow file

0710c44

modify client behaviour for model association list

c375225

add some documentation for writing tests

b3d7f7f

hasan7n force-pushed the server-tests branch from 26507cc to b3d7f7f Compare November 20, 2023 11:10

hasan7n had a problem deploying to testing-external-code November 20, 2023 11:11 — with GitHub Actions Failure

hasan7n added 3 commits November 20, 2023 12:20

update workflow file

2210f69

update server readme

d87a3b1

Merge remote-tracking branch 'upstream/main' into server-tests

f89e706

hasan7n had a problem deploying to testing-external-code November 20, 2023 12:00 — with GitHub Actions Failure

empty

43430f5

hasan7n had a problem deploying to testing-external-code November 20, 2023 16:44 — with GitHub Actions Failure

aristizabal95 reviewed Nov 21, 2023

View reviewed changes

hasan7n added 4 commits November 24, 2023 02:50

change: fix mlcube associations list permissions

a6e3b51

change: modify result PUT and DELETE

f043efa

- added is_valid and user_metadata fields - PUT and DELETE now are only for admin

change: fix permissions of approving the benchmark

239771c

change: prevent updating a benchmark to PENDING

d06ad11

hasan7n added 8 commits November 24, 2023 04:03

Revert "change: remove unused endpoints"

58e505e

This reverts commit 152862a. and deactivates the endpoints

change: restrict marking benchmark as operation

d480dc8

doing this requires its three mlcubes to be operation

change: demo dataset url is required

da4042d

change: fix delete permissions for associations

e460be7

change: server check for dataset assoc prep mlcube

74cedbf

change: client side new association logic

63f6361

change: make client side demo url required

6aa5ebd

cleanup TODOs and related minor changes

a528299

hasan7n had a problem deploying to testing-external-code November 24, 2023 09:13 — with GitHub Actions Failure

hasan7n added 5 commits November 25, 2023 03:08

fix linter issues

6846dd4

use a better datetime parser

eb36d9e

change integration tests order of model priority

c7b6578

stop using benchmark.models for permission reasons

a5ecee2

add Result's new fields to the client

1687b34

hasan7n had a problem deploying to testing-external-code November 25, 2023 07:04 — with GitHub Actions Failure

refactor to fix linter issue

8957464

hasan7n temporarily deployed to testing-external-code November 26, 2023 21:34 — with GitHub Actions Inactive

aristizabal95 approved these changes Nov 27, 2023

View reviewed changes

hasan7n merged commit 8c26674 into mlcommons:main Dec 1, 2023
8 checks passed

github-actions bot locked and limited conversation to collaborators Dec 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server tests #502

Server tests #502

hasan7n commented Nov 19, 2023 •

edited

Loading

github-actions bot commented Nov 19, 2023 •

edited

Loading

aristizabal95 Nov 21, 2023

hasan7n Nov 24, 2023

aristizabal95 left a comment

aristizabal95 Nov 27, 2023

hasan7n Dec 1, 2023

aristizabal95 Nov 27, 2023

aristizabal95 Nov 27, 2023

hasan7n Dec 1, 2023

Server tests #502

Server tests #502

Conversation

hasan7n commented Nov 19, 2023 • edited Loading

github-actions bot commented Nov 19, 2023 • edited Loading

aristizabal95 Nov 21, 2023

Choose a reason for hiding this comment

hasan7n Nov 24, 2023

Choose a reason for hiding this comment

aristizabal95 left a comment

Choose a reason for hiding this comment

aristizabal95 Nov 27, 2023

Choose a reason for hiding this comment

hasan7n Dec 1, 2023

Choose a reason for hiding this comment

aristizabal95 Nov 27, 2023

Choose a reason for hiding this comment

aristizabal95 Nov 27, 2023

Choose a reason for hiding this comment

hasan7n Dec 1, 2023

Choose a reason for hiding this comment

hasan7n commented Nov 19, 2023 •

edited

Loading

github-actions bot commented Nov 19, 2023 •

edited

Loading