Ajout des fonctions et REST endpoints pour gérer les dictionnaires job-utilisateur #186

notoraptor · 2024-03-25T14:08:44Z

Bonjour @soline-b ! Voici une PR pour les fonctions des dictionnaires job-utilisateur ! Dans cette PR:

J'ai ajouté les 3 fonctions pour get, set et delete des user props.
J'ai ajouté 3 entrées dans l'API REST pour permettre l'utilisation de ces fonctions.
J'ai ajouté 3 wrappers au client clockwork_tools pour utiliser les nouvelles entrées de l'API REST.
J'ai ajouté des tests.

gyom · 2024-03-27T17:14:48Z

Je pense qu'on a un problème en quelque part avec les job_id qui sont des str et des int.

Cette PR accepte des int à plusieurs endroits, mais quand je creuse pour voir les fonctions qui sont appelées par get_jobs je tombe vite sur des cas spécifique comme
fake_data.json où on peut voir que les job_id sont de type str.

Il y un endroit dans cette PR qui convertit même en int spécifiquement avant de faire la requête à MongoDB.

result = list(
        mc["job_user_props"].find(
            {
                "job_id": int(job_id),
                "cluster_name": str(cluster_name),
                "mila_email_username": (
                    mila_email_username or current_user.mila_email_username
                ),
            }
        )
    )

Je vois que ça fonctionne pour les tests unitaires parce qu'il n'y a pas d'interaction entre les deux. Il n'y a pas de test présentement qui récupère une job pour un utilisateur en allant chercher simultanément les propriétés que cet utilisateur aurait mis pour la job. Si on avait un test comme ça, on verrait rapidement que

    {
      "mila_email_username": "[email protected]",
      "job_id": 795002,
      "cluster_name": "mila",
      "props": {
        "name": "je suis une user prop 1"
      }
    },

n'est pas compatible avec

{
      "slurm": {
        "account": "mila",
        "cluster_name": "mila",
        "time_limit": 5760,
        "submit_time": 1685759153,
        "start_time": 1685759153,
        "end_time": 1686102142,
        "exit_code": "SUCCESS:0",
        "array_job_id": "0",
        "array_task_id": "None",
        "job_id": "795002",
        "name": "somejobname_732022",
        "nodes": "None assigned",
        "partition": "fun_partition",
        "job_state": "CANCELLED",
        "tres_allocated": {},
        "tres_requested": {
          "num_cpus": 24,
          "mem": 122880,
          "num_nodes": 1,
          "billing": 7,
          "num_gpus": 8
        },
        "username": "milauser06",
        "working_directory": "/a123/b437/c349"
      },
      "cw": {
        "mila_email_username": "[email protected]",
        "last_slurm_update": 1686248611.8868518,
        "last_slurm_update_by_sacct": 1686248611.8868518
      },
      "user": {}
    },

La raison pourquoi on utilisait des str au lieu de int pour les job_id était parce qu'on n'était pas 100% sûrs initialement de comment les job_id fonctionnaient dans Slurm. Il y a des notations avec des "[]" ou des "%", et au lieu de prendre une décision très tôt sur le fait de tout forcer à être en int et le regretter, on a mis des str.

On voudrait peut-être faire un changement éventuellement, mais pour l'instant les job_id sont des int dans notre représentation interne. On veut accepter des appels d'API où ils sont des int ou des str, mais à l'interne on doit être cohérents.

clockwork_web/rest_routes/jobs.py

gyom · 2024-03-28T13:36:42Z

clockwork_web/rest_routes/jobs.py

+    updates = request.values.get("updates", None)
+    if updates is None:
+        return jsonify(f"Missing argument 'updates'."), 500
+    elif isinstance(updates, str):


On ne devrait pas faire json.loads sur une string passée par l'utilisateur. Jadis c'était dangereux de faire cela, mais là on dirait que la documentation de Python est moins inquiétante à cet égard.

Dans tous les cas, une bonne utilisation de l'interface REST mène à ce qu'on ait un objet dictionnaire à cet endroit et pas une string à décoder en JSON.

Si updates n'est pas un dict, on retourne un message d'erreur.

Je propose une solution ici: 5d29008

clockwork_web/rest_routes/jobs.py

gyom · 2024-03-28T13:42:58Z

clockwork_web/rest_routes/jobs.py

+    # Set props, using current_user_id as mila email username.
+    try:
+        set_user_props(job_id, cluster_name, updates, current_user_id)
+        props = get_user_props(job_id, cluster_name, current_user_id)


Ça semble être tout innocent de retourner les nouvelles propriétés tant qu'à y être, mais ça va quand même produire une nouvelle requête tout aussi lourde que la première pour faire le set.

On peut faire ça parce que ça pourrait être commode pour l'utilisateur, mais il faudrait au lieu passer par

new_props = set_user_props(job_id, cluster_name, updates, current_user_id) return jsonify(new_props)

après avoir mis à jour la fonction set_user_props pour qu'elle retourne ces valeurs (qu'elle a déjà sous la main sans coûts additionnels).

Fait! 1955a3a

gyom · 2024-03-28T13:44:09Z

clockwork_web/rest_routes/jobs.py

+        return jsonify(props)
+    except ValueError as exc:
+        # If props size limit error occurs, return it as an HTTP 500 error.
+        return jsonify(str(exc)), 500


On ne veut pas retourner de stack trace à l'utilisateur, pour des raisons de sécurité.

Fait! 64fc63b

gyom · 2024-03-28T13:50:50Z

clockwork_web/rest_routes/jobs.py

+    keys = request.values.get("keys", None)
+    if keys is None:
+        return jsonify(f"Missing argument 'keys'."), 500
+    elif isinstance(keys, str):


Ce n'est pas à nous de charger du JSON fourni par l'utilisateur. On se fie à la sous-librairie de Flask pour faire ça automatiquement (et de manière sécuritaire) quand un objet JSON est fourni.

En fait, si l'utilisateur nous envoie une seule string, je préfère qu'on la traite en la mettant dans une liste d'un seul élément. C'est déjà une chose qui est gérée par l'argument key_or_keys de la fonction delete_user_props alors on peut en profiter.

Je propose une solution ici: 5d29008

gyom · 2024-03-28T13:51:11Z

clockwork_web/rest_routes/jobs.py

+        try:
+            keys = json.loads(keys)
+        except Exception as exc:
+            return jsonify(f"Failed to json.loads(keys). \n{exc}."), 500


D'ailleurs, on ne passe pas de stack trace en retour.

Fait! 64fc63b

gyom · 2024-03-28T13:57:39Z

clockwork_web/rest_routes/jobs.py

+
+    # Delete props, using current_user_id as mila email username.
+    delete_user_props(job_id, cluster_name, keys, current_user_id)
+    props = get_user_props(job_id, cluster_name, current_user_id)


Même commentaire que précédemment concernant l'opération d'extra. Cette opération double essentiellement les coûts pour très peu de bénéfice. On a déjà les nouvelles propriétés en main, alors pourquoi vouloir appeler la fonction get_user_props?

Dans le cas de delete_user_props, en plus, je pense que ce n'est pas vraiment attendu par l'utilisateur qu'on aille lui fournir la liste des propriétés restantes. On peut probablement retourner rien du tout ici, ou sinon ceci:

return jsonify(f"Successfully deleted keys {keys}."), 200

Fait! cb0d2fd

clockwork_web_test/test_core_job_user_props_helper.py

Allow to pass username to job-user functions (default is still current logged user).

…rops. Add unit tests.

…d client.

notoraptor · 2024-04-03T13:03:24Z

#186 (comment)

@gyom pour ce commentaire relatif aux job IDs, ils sont désormais tous gérés en tant que strings: #186 (comment)

notoraptor · 2024-04-03T13:22:26Z

PR à jour ! Il reste encore la gestion des dicts et des lists dans l'API REST. Je fais encore quelques recherches à ce sujet, et je ferai un commit ou un commentaire dans les prochaines heures.

…equests.

notoraptor · 2024-04-03T14:30:58Z

PR à jour! @gyom @soline-b

Je ne sais pas si ça correspond exactement à ce que voulait @gyom , mais j'ai constaté qu'on pouvait envoyer des requêtes HTTP qui sont directement en JSON, avec le header Content-type: application/json. Le module Python requests accepte alors d'envoyer de telles requêtes et d'encoder lui-même le dictionnaire en JSON (tant que le dictionnaire est effectivement sérialisable en JSON).

Côté Flask, on peut vérifier si la requête reçue est en JSON avec request.is_json, et récupérer le JSON avec request.get_json(). C'est alors ce JSON qui doit contenir tous les paramètres attendus par la requête. Par exemple, pour set_user_props, on attend les champs job_id (str), cluster_name (str) et updates (dict, donc un dict dans un dict).

Commit: 5d29008

PS: Les tests ne roulent pas parce que le CI ne trouve pas docker-compose. Pourtant, je n'ai rien changé ...

…delete_user_props

notoraptor · 2024-04-03T15:06:31Z

C'est bon, les tests passent !

gyom · 2024-04-04T14:11:04Z

clockwork_tools_test/test_mt_job_user_props.py

+
+
+@pytest.fixture
+def client(config, db_with_fake_data):


Le client existe déjà dans les fixtures communes aux tests de clockwork_tools. Il s'appelle mtclient parce qu'initialement j'avais appelé ceci "Mila tools", d'où le "mtclient". Ensuite de ça, Olivier a appelé un autre outil "Mila tools" alors le nom n'était plus disponible. Le préfixe "mt" est resté à certains endroits.

Le point c'est que tu devrais éviter de faire un nouveau client pour ces tests alors qu'il existe déjà cela dans conftest.py dans le même répertoire. Le fichier test_mt_jobs.py illustre bien comment s'en servir.

En gros, tu peux simplement passer la fixture mtclient tout bonnement à tes tests au lieu de passer ta fixture client (et tu peux effacer celle-ci). L'utilisateur par défaut pour les tests est "[email protected]", mais je ne crois pas que tes tests dépendent de celui-ci.

Bonjour, Guillaume ! En fait, dans mes tests, c'est student00 qui définit des user props, pas student01. C'est pour ça que j'avais besoin d'un nouveau client.

Mais je pourrais changer l'utilisateur qui crée les user props dans fake data en passant de student00 à student01. Comme ça je pourrai utiliser mtclient. Est-ce que je fais comme ça ?

Oui, mets les valeurs à student01 tout simplement et ça évite d'avoir à créer un autre test fixture.

gyom · 2024-04-04T14:23:15Z

clockwork_tools/clockwork_tools/client.py

@@ -60,7 +60,7 @@ def _get_headers(self):
        encoded_s = str(encoded_bytes, "utf-8")
        return {"Authorization": f"Basic {encoded_s}"}

-    def _request(self, endpoint, params, method="GET"):
+    def _request(self, endpoint, params, method="GET", send_json=False):


Dans la perspective où on va toujours envoyer du JSON parce que c'est la seule chose que le serveur accepte, je vois mal pourquoi on voudrait avoir send_json=False par défaut. Autant le mettre à True, et la seule raison pourquoi on permettrait qu'il soit même possible d'envoyer autre chose que du JSON serait au cas où on voudrait ajouter cette feature dans le futur.

Dans ce cas, je serais même tenté de changer la fonction pour enlever cet argument et laisser le code en commentaire comme indice pour le futur si on veut faire un tel changement.

elif method == "PUT": headers = self._get_headers() headers["Content-type"] = "application/json" response = requests.put(complete_address, json=params, headers=headers) # If we ever decide not to send JSON content, the syntax would be the following: # response = requests.put( complete_address, data=params, headers=self._get_headers() )

Il y a une requête PUT qui n'envoie pas ses paramètres en JSON, c'est:

"api/v1/clusters/jobs/user_dict_update"

Elle est défnie ici: https://github.com/mila-iqia/clockwork/blob/master/clockwork_web/rest_routes/jobs.py#L129

Cela dit, j'ai l'impression que c'est une requête qui était destinée à être enlevée, précisément dès que les user props seraient implémentées. Donc, si j'enlève cette requête, alors je pourrai partir du principe que toutes les requêtes PUT sont envoyées entièrement en JSON.

Est-ce que je l'enlève ?

Il faudrait enlever ces anciennes fonctionnalités, mais quant à savoir si ta PR est le bon endroit pour l'enlever ou pas, je ne pense pas que ça l'est.

Voici ce que je proposerais. Tu peux mettre send_json=True comme valeur par défaut et modifier la fonction originale de api/v1/clusters/jobs/user_dict_update pour qu'elle ajoute l'argument send_json=False explicitement. Comme ça, les fonctions déjà présentes ne seront pas cassées, et il sera un peu plus clair qu'on se dirige vers un interface où tout est passé en json.

Le ticket éventuel consistant à enlever ces anciennes fonctionnalités de api/v1/clusters/jobs/user_dict_update pourra ensuite enlever au complet l'argument send_json qui n'aura plus d'utilité. Avec cette approche, on sépare un peu mieux les étapes au lieu de tout lancer dans une même PR.

Est-ce que ça te semble bon comme approche?

gyom · 2024-04-04T14:56:00Z

Je regarde les tests et je constate qu'il y a un patron d'utilisation qui n'est pas vraiment celui qu'on souhaiterait avoir.

Voici un exemple de test qui conviendrait. Je pense que tu pourras mieux voir comment l'approche diffère de celle présentement dans "test_mt_job_user_props.py". Il se peut que j'aie des erreurs dans le code que je vais fournir parce que je ne peux pas l'insérer directement dans ta PR.

def test_cw_tools_get_user_props(mtclient, fake_data):
    """
    This test relies on the presence of at least one job_user_props entry in the fake data
    that belongs to the user that's currently logged in for `mtclient` (which is "[email protected]").
    """

    # Find an entry that's associated with the user that's currently logged in.
    # This becomes the ground truth against which we compare the retrieved user props.
    LD_candidates = [
            D_job_user_props_entry
            for D_job_user_props_entry in fake_data["job_user_props"]
            if (D_job_user_props_entry["mila_email_username"] == mtclient.email
                and len(D_job_user_props_entry["props"]) > 0)
        ]
    assert len(LD_candidates) > 0, "There should be at least one job_user_props entry for the user that's currently logged in."
    original_D_job_user_props_entry = random.choice(LD_candidates)

    job_id = D_job_user_props_entry["job_id"]
    cluster_name = D_job_user_props_entry["cluster_name"]
    original_props = D_job_user_props_entry["props"]

    # Use the client to retrieve the user props.
    retrieved_props = mtclient.get_user_props(job_id, cluster_name)

    assert len(retrieved_props) == len(original_props), "The number of user props should be the same."
    for k, v in original_props.items():
        assert retrieved_props[k] == v, f"Expected {k} to be {v} but got {retrieved_props[k]}."

La fixture fake_data est vraiment là pour pouvoir connaître le contenu de la base de données au moment de faire un test qui récupère de l'information par l'interface à tester. On peut évidemment transcrire manuellement le contenu de "fake_data.json" dans le test si c'est pour peu de choses, mais là on s'empêtre dans l'histoire de comment la génération de fake data fonctionne. C'est beaucoup plus simple si le test est capable lui-même de juste savoir ce qui est dans la base de données, ce qui représente la référence en terme de la bonne réponse à obtenir. Ça permet de tester avec beaucoup plus de variété de données que si on avait choisi manuellement une seule valeur.

Update user props tests, do not use useless new fixtures anymore

notoraptor · 2024-04-05T19:56:52Z

@gyom PR à jour !

Commit pour les tests: db99cd5

Commit pour send_json=True par défaut: 6914b2f

Il y a un 3e commit juste pour corriger les tests du frontend.

gyom · 2024-04-08T14:31:17Z

scripts/store_huge_fake_data_in_db.py

@@ -215,7 +215,7 @@ def _generate_huge_fake_data(
    job_user_dicts = [
        {
            "mila_email_username": props_username,


L'utilisateur pour tester est [email protected], tel que défini dans la configuration, mais tu es un peu en train de hardcoder ici que ce soit [email protected] par le fait que ce soit un argument par défaut. Est-ce que la valeur de la configuration est passée à ce script ailleurs?

D'ailleurs, à moyen terme on voudrait probablement avoir une plus grande diversité de user props que ça au lieu d'avoir l'utilisateur de test qui a toutes les entrées. Peut-être que c'est mieux de reporter ça à une prochaine PR, mais on va vouloir avoir le genre de user props qui s'affichent dans l'interface pour les jobs d'un utilisateur qui regarde ses propres jobs. Je ne me souviens plus la convention qu'on s'était donnés, mais je parle ici des choses à la "wandb_project" ou "wandb_name" (on reparlera de la convention plus tard).

Il y aurait manière de mettre que 80% des entrées dans job_user_dict appartienne au propriétaire d'une job, et les autres soient éparpillées.

Je pense que je n'avais pas compris que ce script n'est pas appelé nulle part durant le setup des tests.

Est-ce que c'est un script utilisé pour tester manuellement certains aspect du benchmarking mais qui ne devrait pas être intégré aux tests unitaires? Ça semble davantage être ça, parce que s'il était ajouté à des endroits comme

python3 scripts/store_fake_data_in_db.py python3 -m flask run --host="0.0.0.0"

cela poserait un gros problème du fait que le contenu de "fake_data.json" ne serait plus du tout synchronisé avec le contenu réel de la base de données (qui aurait été alimenté par ce script "store_huge_fake_data_in_db.py").

Mais si c'est juste un script pour faire des benchmarks, alors c'est correct. Il faudrait peut-être l'indiquer plus clairement dans le commentaire au début du script. Enfin, il y a des explications qui sont bonnes à l'heure actuelle, mais quelqu'un pourrait facilement être induit en erreur en pensant que c'est pris en charge par les test fixtures aussi bien que ce qui se passe pour "store_fake_data_in_db.py".

Ce script store_huge_fake_data_in_db n'est effectivement pas utilisé dans le code actuel de clockwork. Il s'agit d'un vestige des benchmarks réalisés il y a quelques semaines. Je l'avais laissé dans le code ( PR #185 ) car je m'étais dit qu'il pouvait être utile à l'avenir, car il permet d'insérer rapidement de grosses quantités de données dans la base de données. Ça peut servir notamment pour de futurs benchmarks !

Mais comme il est actuellement inutile, j'ai fait un commit pour l'enlever: cf4e345

Si besoin, je pourrai faire une PR séparée pour le remettre. Ou bien le laisser dans une branche à part, sur mon dépôt, et le réutiliser seulement si nécessaire!

test_common/fake_data.json

gyom · 2024-04-08T15:00:34Z

clockwork_web_test/test_core_job_user_props_helper.py

+
+def test_get_user_props(app, client, fake_data):
+    # Log in to Clockwork
+    mila_email_username = "[email protected]"


Pourquoi est-ce que tes tests doivent manuellement faire les logins eux-mêmes alors que les tests dans "test_core_jobs_helper.py" n'en ont pas besoin?

C'est parce que les fonctions job-user-props (donc get_user_props, set_user_props et delete_user_props) vont chercher l'email de l'utilisateur actuellement connecté si aucun email ne leur est fourni dans leurs paramètres. Or, ces tests-ci appellent les fonctions en leur donnant seulement le job_id et le cluster_name. Les fonctions ont donc besoin qu'un utilisateur soit connecté, pour récupérer l'email. Si non, elles crashent. Sans login manuel, le current_user n'est pas disponible (il vaut None).

Par exemple, dans ce test, on appelle get_user_props(job_id=job_id, cluster_name=cluster_name). Le paramètre mila_email_username n'est pas fourni, donc il est à None par défaut, donc la fonction ira chercher le current_user.mila_email_username pour connaître l'email de l'utilisateur pour lequel elle doit retourner les user props.

Je peux retirer les logins manuels, mais alors je devrai juste passer explicitement le mila_email_usernameaux fonctions. Mais je supposais qu'on voulait que l'email de l'utilisateur puisse être implicite.

Après discussion:

les tests core ont été retirés pour ces fonctions, car elles ont besoin du contexte d'un utilisateur connecté pour s'exécuter correctement. Elles sont donc indirectement testées via les tests de l'API REST.

Les fonctions doivent désormais recevoir explicitement le mila_email_username. Elles sont donc considérées comme des fonctions internes (à ne pas exposer à un utilisateur). Un utilisateur devrait seulement se servir de clockwork tools, ou faire un appel via l'API REST.

Commit: 71fca56

gyom · 2024-04-08T15:19:29Z

clockwork_web_test/test_rest_job_user_props.py

+
+
+def _get_test_user_props(fake_data):
+    email = "[email protected]"


Quelque chose me dit que ce n'est pas la bonne manière de faire pour avoir ce email qui vient de la configuration, mais sur le coup je ne suis pas sûr quoi suggérer.

Est-ce que le test lui-même a accès aux valeurs de get_config('clockwork.test.email') ?

L'enjeu ici c'est qu'on cherche à faire une requête

bien authentifiée par valid_rest_auth_headers,

qui récupère les propriétés de jobs de l'utilisateur dont les headers précèdent,
mais ça veut dire qu'on aurait besoin de savoir de quel utilisateur proviennent ces headers valides.

Bon, y'aurait manière de faire une passe un peu maladroite pour simplement décoder le contenu de
{"Authorization": f"Basic {encoded_s}"} mais c'est un peu bizarre.

On en parlera mardi avec @soline-b.

Apparemment oui, le test a accès à get_config(), donc on peut utiliser cette fonction (au lieu de hardcoder l'email). Corrigé ici: 71fca56

…te_user_props Remove core tests for these functions, as they must be called in a context with a valid logged user

soline-b · 2024-04-17T14:08:24Z

clockwork_frontend_test/test_settings.py

+    page.goto(f"{BASE_URL}/settings/")
+    # Get language select.
+    select = page.locator("select#language_selection")
+    # Check default language is english.


Minor typo in the comments :

This one should be "Check that default language is French"

and the following should be "Switch to English"

Fixed! 041834e

clockwork_tools_test/test_mt_job_user_props.py

gyom · 2024-04-17T15:00:49Z

clockwork_web/core/job_user_props_helper.py

+        db.insert_one(
+            {
+                "job_id": job_id,
+                "cluster_name": str(cluster_name),


C'est bizarre qu'on veuille faire str(cluster_name) ici. C'est une fonction interne alors on devrait avoir testé antérieurement que le cluster_name était une string.

Si on ne fait pas confiance à cette valeur pour être une string, alors il fallait produire un message d'erreur avant ça. Rendus ici, on devrait quasiment plus rejeter l'appel ou quelque chose si on a quoique ce soit d'autre qu'une string pour le cluster_name. Par ailleurs, pourquoi ferions-nous cela pour cluster_name et pas pour job_id aussi?

La réponse c'est que l'endroit pour faire la conversion de job_id en string (au cas où un utilisateur aurait passé un int) est bien avant cet appel. Enfin, quand je vois un str(cluster_name), je ne vois pas ça comme une précaution inoffensive, mais je vois ça comme une valeur qui n'a pas été "sanitized" au bon endroit au préalable.

En effet, c'est une précaution excessive et inutile. Retirée: 73477ee

gyom · 2024-04-17T15:02:32Z

clockwork_web/core/job_user_props_helper.py

+        mc["job_user_props"].find(
+            {
+                "job_id": job_id,
+                "cluster_name": str(cluster_name),


Même commentaire qu'avant. Comment est-ce qu'une valeur de cluster_name non-string pourrait se rendre là? Si c'est une chose possible, il faudrait tester la valeur fournie par l'utilisateur avant pour pouvoir lui renvoyer un message d'erreur, et ce n'est pas la responsabilité de cette fonction-ci d'envoyer un message d'erreur. Ça se passe à un niveau plus haut.

Retirée, ce n'est en effet pas nécessaire: 73477ee

gyom · 2024-04-17T15:34:36Z

clockwork_web/rest_routes/jobs.py

+    try:
+        props = set_user_props(job_id, cluster_name, updates, current_user_id)
+        return jsonify(props)
+    except ValueError:


J'avais déjà dit que je ne voulais pas qu'on retourne de stack trace, alors c'est bien de ne pas en retourner. Il y a par contre un message ici qui est très spécifique mais qui n'est pas vraiment fourni par la ValueError.

Bon, pour l'instant on n'a qu'un seul type d'erreur retourné, donc tu supposes que c'est celui-là. C'est juste, mais on pourrait aussi avoir d'autres erreurs possibles venant de MongoDB et tout ça et l'utilisateur se ferait dire "hey, tes props étaient trop grosses!" dans ce cas.

En fait, le ValueError généré par set_user_props est très bien étoffé avec des détails, et tout cela se perd. Il y a manière de récupérer ce texte et s'en servir comme feedback à l'utilisateur sans toutefois fournir le stack trace complet (ce qu'on veut éviter).

D'ailleurs, on n'a pas de cas pour traiter une situation où un erreur dans l'appel à MongoDB a eu lieu, ou quoique ce soit d'autre. On pourrait avec une clause de except pour toutes les autres sortes d'erreur, et juste dire à l'utilisateur qu'un type d'erreur non-spécifique est arrivé sur le serveur.

Fait ! 87a285a

J'ai défini une exception spécifique pour la taille limite des user props, je la capture à part, et je capture aussi les autres exceptions.

Quand l'exception de la taille des user props est capturée, son message est affiché dans la réponse REST (simplement avec str(exception), qui va juste afficher le message de l'exception, sans aucune autre stack trace). Il y a un test qui vérifie le message, et qui est aussi mis à jour dans ce commit.

…ion error message in REST response.

gyom · 2024-04-18T14:20:48Z

clockwork_web_test/test_rest_job_user_props.py

+def test_size_limit_for_jobs_user_props_set(client, valid_rest_auth_headers, fake_data):
+    job_id, cluster_name, original_props = _get_test_user_props(fake_data)
+    assert "other  name" not in original_props
+    huge_text = "x" * (2 * 1024 * 1024)


Pourrais-tu référer à clockwork_web.core.job_user_props_helper.MAX_PROPS_LENGTH au lieu de mettre un nombre magique ici?

Ça devrait devenir un paramètre de configuration éventuellement, mais en attendant ça serait bon de référer à cette variable au lieu de la hardcoder à deux endroits.

Fait! b28ebb0

Mis à jour! 63a8503

gyom reviewed Mar 27, 2024

View reviewed changes

clockwork_web/rest_routes/jobs.py Outdated Show resolved Hide resolved

gyom reviewed Mar 28, 2024

View reviewed changes

clockwork_web/rest_routes/jobs.py Outdated Show resolved Hide resolved

gyom reviewed Mar 28, 2024

View reviewed changes

clockwork_web_test/test_core_job_user_props_helper.py Outdated Show resolved Hide resolved

notoraptor added 9 commits April 2, 2024 08:48

Add functions to get, set and delete job-user props.

aa20f4e

Add routes to manage job-user dicts.

5a8c319

Allow to pass username to job-user functions (default is still current logged user).

Add clockwotk tools functions to call REST APIs related to job-user p…

ed6ea1b

…rops. Add unit tests.

Set job_id as a string in mongodb collection job_user_props

db78f6d

Do not print exceptions in error message returned by REST API.

64fc63b

Update REST API error messages.

e89a5df

Make set_user_props() return new updated props.

1955a3a

Make REST API return nothing when deleting user props.

cb0d2fd

Remove useless new pytest fixtures, just use already available app an…

7bfc643

…d client.

notoraptor force-pushed the job-user-dict-functions branch from 1df3b2e to 7bfc643 Compare April 3, 2024 12:57

notoraptor added 2 commits April 3, 2024 10:18

Remove unused imports.

a626be7

Make API endpoints set_user_props and delete_user_props as JSON PUT r…

5d29008

…equests.

notoraptor added 2 commits April 3, 2024 10:42

Replace docker-compose with docker compose

ec1c4b4

Make sure to send JSON requests in REST tests for set_user_props and …

11e3d78

…delete_user_props

gyom reviewed Apr 4, 2024

View reviewed changes

notoraptor added 3 commits April 5, 2024 15:23

In fake_data, create user props with student01 instead of student00

db99cd5

Update user props tests, do not use useless new fixtures anymore

Set send_json=True by default for clockwork tool method _request().

6914b2f

Fix frontend tests for user props: login using student01

4426490

gyom reviewed Apr 8, 2024

View reviewed changes

test_common/fake_data.json Show resolved Hide resolved

gyom reviewed Apr 8, 2024

View reviewed changes

Remove irrelevant script: store_huge_fake_data_in_db

cf4e345

gyom reviewed Apr 8, 2024

View reviewed changes

Make mila_email_username mandatory in Python functions get/set/dele…

71fca56

…te_user_props Remove core tests for these functions, as they must be called in a context with a valid logged user

soline-b reviewed Apr 17, 2024

View reviewed changes

gyom reviewed Apr 17, 2024

View reviewed changes

clockwork_tools_test/test_mt_job_user_props.py Show resolved Hide resolved

gyom reviewed Apr 17, 2024

View reviewed changes

notoraptor added 2 commits April 17, 2024 11:28

Fix comments.

041834e

Remove useless str conversions on cluster_name.

73477ee

gyom reviewed Apr 17, 2024

View reviewed changes

notoraptor added 2 commits April 17, 2024 11:52

Simplify tests for user props in clockwork_tools.

a81a073

Use a specific exception for huge user props error, and return except…

87a285a

…ion error message in REST response.

gyom reviewed Apr 18, 2024

View reviewed changes

notoraptor added 2 commits April 18, 2024 10:47

Import user props size limit in tests, instead of hardcoding x1again.

b28ebb0

In unit tests, make huge prop a little larger than size limit.

63a8503

soline-b merged commit d31e544 into mila-iqia:master May 9, 2024
2 checks passed

notoraptor deleted the job-user-dict-functions branch May 14, 2024 15:21



		def _get_test_user_props(fake_data):
		email = "[email protected]"

Ajout des fonctions et REST endpoints pour gérer les dictionnaires job-utilisateur #186

Ajout des fonctions et REST endpoints pour gérer les dictionnaires job-utilisateur #186

Conversation

notoraptor commented Mar 25, 2024

gyom commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyom Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

notoraptor commented Apr 3, 2024

notoraptor commented Apr 3, 2024

notoraptor commented Apr 3, 2024

notoraptor commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyom commented Apr 4, 2024 • edited Loading

notoraptor commented Apr 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyom Mar 28, 2024 •

edited

Loading

gyom commented Apr 4, 2024 •

edited

Loading