feat(api): add possibility to run study simulations #35

salemsd · 2024-12-12T15:28:20Z

Studies now have a run service that can be called to both start a simulation and wait for its job
Unit tests are working

MartinBelthle

Nice work :) Once the comments will be resolved you could add an integration test it would be great

MartinBelthle · 2024-12-13T10:48:20Z

src/antares/model/study.py

+
+        Returns: A job representing the simulation task
+        """
+        return self._run_service.run_antares_simulation()


You should pass the parameters here

MartinBelthle · 2024-12-13T10:50:27Z

src/antares/model/study.py

+        """
+        return self._run_service.run_antares_simulation()
+
+    def wait_job_completion(self, job: Job, time_out: int) -> None:


time_out should be optional IMO. With a really high default value if not given such as 172800 (default value in AntaresWeb)

MartinBelthle · 2024-12-13T10:51:02Z

src/antares/model/study.py

+        """
+        Runs the Antares simulation.
+
+        This method starts an antares simulation for the current study config and params


Could be This method starts an antares simulation with the given parameters

MartinBelthle · 2024-12-13T10:52:28Z

src/antares/model/settings/solver.py

+from enum import Enum
+
+
+class Solver(Enum):


I think you should put this class at the same place as the job one and simulation_parameters, inside a simulation.py file
As settings is for the study generaldata.ini file

MartinBelthle · 2024-12-13T10:54:31Z

src/antares/model/settings/antares_simulation_parameters.py

+        if self.presolve:
+            options.append("presolve")
+        if self.solver != Solver.SIRIUS:
+            options.append(self.solver.name.lower())


self.solver.name.lower() could be self.solver.value as it's already in lower case

MartinBelthle · 2024-12-13T11:07:29Z

src/antares/service/api_services/run_api.py

+            job.status = updated_job.status
+            job.unzip_output = updated_job.unzip_output
+
+        if job.unzip_output:


Once you go out of the loop your status could be success or failed.
If it's failed, you should raise an Exception to say the simulation failed.
Else you can continue your code

MartinBelthle · 2024-12-13T11:10:08Z

src/antares/service/api_services/run_api.py

+
+        return None
+
+    def _unzip_output(self, ref_id: str, type: list[str]) -> None:


Perhaps my spec wasn't really clear as it's a complex subject but what you need to do in this method is to wait for the output to unzip itself. So you can rename this method.
Also, your payload should simply be

{ "type": type, "ref_id": ref_id }

otherwise you won't have any result (example: to_completion_date: 0 means you want all jobs that ended before date 0 which will return no jobs)

MartinBelthle · 2024-12-13T11:11:38Z

src/antares/service/api_services/run_api.py

+                "from_completion_date_utc": 0,
+                "to_completion_date_utc": 0,
+            }
+            self._wrapper.post(url, json=payload)


Finally here you should repeat this call every 2 seconds for example, and check the output to see if the task ended successfully. I can reexplain this to you if you need

MartinBelthle · 2024-12-13T11:12:46Z

src/antares/exceptions/exceptions.py

+
+
+class SimulationTimeOutError(Exception):
+    def __init__(self, job_id: str, time_out: int, message: str = "Error") -> None:


Don't really see the point of the 'message' argument here

MartinBelthle · 2024-12-13T11:14:53Z

tests/antares/services/api_services/test_study_api.py

+            mocker.post(run_url, json={"id": job_id}, status_code=200)
+
+            job_url = f"https://antares.com/api/v1/launcher/jobs/{job_id}"
+            response_list = [


I'm not sure to understand this. The output shouldn't be a list when calling this endpoint. We can discuss it later.

MartinBelthle · 2024-12-13T11:20:11Z

src/antares/model/settings/antares_simulation_parameters.py

+            options.append(self.solver.name.lower())
+        return " ".join(options)
+
+    def model_dump(self, *args: Any, **kwargs: Any) -> Dict[str, Any]:


We should rename this method to_api() instead of model_dump because when we'll implement the local method we'll have to parse the object differently and it would be clearer

MartinBelthle

Some comments but we're in the right direction :)

As we discussed you can introduce a test inside test_web_client that just launches a simulation and asserts the job succeeded

MartinBelthle · 2024-12-13T16:19:50Z

src/antares/service/api_services/run_api.py

+        response = self._wrapper.get(url)
+        job_info = response.json()
+        status = JobStatus.from_str(job_info["status"])
+        output_id = job_info["output_id"] if status == JobStatus.SUCCESS else None


I think you can just use job_info["output_id"] as it will be None if the job succeeded

job_info["output_id"] doesn't even exist before the job succeeds, so it will throw a KeyError

Okay. Could be output_id = job_info.get("output_id") then ?

MartinBelthle · 2024-12-13T16:21:46Z

src/antares/service/api_services/run_api.py

+        if job.status == JobStatus.FAILED:
+            raise SimulationFailedError(self.study_id)
+
+        if job.unzip_output and job.output_id:


I agree here you should assert that we have an output_id. But if we haven't it seems to me we should raise a SimulationFailedError. So i think you should put this as a or not job.output_id line 71 and remove it from this line

MartinBelthle · 2024-12-13T16:22:39Z

src/antares/service/api_services/run_api.py

+
+        return None
+
+    def _wait_unzip_output(self, ref_id: str, type: list[str], job_output_id: str) -> None:


Actually we shouldn't give ["UNARCHIVE"] to this method, we should use it internally as we cannot call this method with another task_type

MartinBelthle · 2024-12-13T16:25:43Z

tests/antares/services/api_services/test_study_api.py

+                    "json": {
+                        "id": job_id,
+                        "status": "running",
+                        "launcher_params": dumps(parameters.to_api()),


You can store dumps(parameters.to_api()) inside a variable instead of calling it 4 times

tests/antares/services/api_services/test_study_api.py

src/antares/service/api_services/run_api.py

MartinBelthle · 2024-12-13T16:39:36Z

src/antares/service/api_services/run_api.py

+        except APIError as e:
+            raise AntaresSimulationUnzipError(self.study_id, e.message) from e
+
+    def _get_task_id(self, job_output_id: str, tasks: list[dict[str, Any]]) -> str:


We could be more specific, like _get_unarchiving_task_id as this code will only work for this type of task and we might one day introduce a get_task_id somewhere in the code that will be more generic

MartinBelthle · 2024-12-13T16:45:21Z

src/antares/service/api_services/run_api.py

+        raise AntaresSimulationUnzipError(self.study_id, "Could not find task for unarchiving job")
+
+    def _get_task_until_success(self, url: str, repeat_interval: int) -> None:
+        task_success = False


This task can fail. And if it does you will never go out of this while. You can introduce a more generic method that has a task_id as an argument and builds its url internally. it could be called wait_task_completion and will have the same behavior as wait_job_completion: a timeout and raise a Timeout error etc.. (For now you can make it private but we'll need this method at some point).

Also task["result"] can return None so you'll have to handle this as a sign it's still running to avoid KeyErrors

MartinBelthle · 2024-12-13T16:48:04Z

tests/antares/services/api_services/test_study_api.py

+    def test_run_and_wait_antares_simulation(self):
+        parameters = AntaresSimulationParameters(solver=Solver.COIN, nb_cpu=2, unzip_output=True, presolve=False)
+
+        # patch simulates the repeating intervals so that we don't have to wait X seconds during the tests


MartinBelthle

Minor changes, mainly a spec change I just came up with :/

src/antares/service/api_services/run_api.py

src/antares/model/simulation.py

tests/integration/test_web_client.py

src/antares/service/api_services/run_api.py

MartinBelthle

Almost done :)

MartinBelthle · 2024-12-16T16:28:44Z

src/antares/service/api_services/run_api.py

+        response = self._wrapper.get(url)
+        job_info = response.json()
+        status = JobStatus.from_str(job_info["status"])
+        output_id = job_info["output_id"] if status == JobStatus.SUCCESS else None


Okay. Could be output_id = job_info.get("output_id") then ?

MartinBelthle · 2024-12-16T16:30:41Z

src/antares/service/api_services/run_api.py

+            self._update_job(job)
+
+        if job.status == JobStatus.FAILED or not job.output_id:
+            raise SimulationFailedError(self.study_id)


I think we should also log the job_id inside this Exception for the user and debugging purpose

MartinBelthle · 2024-12-16T16:33:06Z

src/antares/service/api_services/run_api.py

+            tasks = response.json()
+            task_id = self._get_unarchiving_task_id(job, tasks)
+            self._wait_task_completion(task_id, repeat_interval, time_out)
+        except APIError as e:


I think you can simply do a

except Exception as e: raise AntaresSimulationUnzipError(self.study_id, job.job_id, e.message) from e

as I think we want to raise this issue in both cases

MartinBelthle requested changes Dec 13, 2024

View reviewed changes

MartinBelthle reviewed Dec 13, 2024

View reviewed changes

salemsd requested a review from MartinBelthle December 13, 2024 16:13

MartinBelthle requested changes Dec 13, 2024

View reviewed changes

salemsd added 17 commits December 16, 2024 14:09

feat(api): set up new modules

3944a78

feat(api): add service in factory

1424109

feat(api): add run simulation method skeleton

70673ac

feat(api): complete run simulation api method

cc70ba0

feat(api): add wait job skeleton

0844568

feat(api): complete wait job method

0e61eb0

feat(api): add unzip support for wait job

1e99657

feat(api): add parameters support for run method

97079e6

feat(api): add test and fix job not changing after wait

672481f

feat(api): rebase

204f7b6

feat(api): remove StrEnum not supported <py3.11

f6d87a8

feat(api): use pydantic for params and update tests

471279f

feat(api): reformat

dfcfb3d

feat(api): replace to_json by model_dump

198b82e

feat(api): add output_id to job, reorganize and fix wait_unzip

9f3407b

feat(api): fixed enum, tests and added patch

3a6d58c

feat(api): fixed typiung errors

5ccf8c7

salemsd force-pushed the feat/run_studies branch from 9f88c9d to 5ccf8c7 Compare December 16, 2024 13:09

feat(api): add integration tests, fix some inconsistencies with run_api

33131b3

salemsd force-pushed the feat/run_studies branch from 9d4bcf3 to 33131b3 Compare December 16, 2024 13:13

feat(api): fix matrix columns and integration test

9c36971

salemsd requested a review from MartinBelthle December 16, 2024 13:57

feat(api): removed unnecessary parameter from test

1e24160

MartinBelthle requested changes Dec 16, 2024

View reviewed changes

feat(api): refactor job and exception handling

2922b2e

salemsd requested a review from MartinBelthle December 16, 2024 15:52

MartinBelthle requested changes Dec 16, 2024

View reviewed changes

feat(api): fix

477557d

salemsd requested a review from MartinBelthle December 16, 2024 16:39

MartinBelthle approved these changes Dec 16, 2024

View reviewed changes

MartinBelthle merged commit 3ca68f4 into main Dec 16, 2024
8 checks passed

MartinBelthle deleted the feat/run_studies branch December 16, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): add possibility to run study simulations #35

feat(api): add possibility to run study simulations #35

salemsd commented Dec 12, 2024

MartinBelthle left a comment

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024 •

edited

Loading

MartinBelthle left a comment

MartinBelthle Dec 13, 2024

salemsd Dec 16, 2024

MartinBelthle Dec 16, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle Dec 13, 2024

MartinBelthle left a comment

MartinBelthle left a comment

MartinBelthle Dec 16, 2024

MartinBelthle Dec 16, 2024

MartinBelthle Dec 16, 2024


		return None

		def _unzip_output(self, ref_id: str, type: list[str]) -> None:



		class SimulationTimeOutError(Exception):
		def __init__(self, job_id: str, time_out: int, message: str = "Error") -> None:


		return None

		def _wait_unzip_output(self, ref_id: str, type: list[str], job_output_id: str) -> None:

feat(api): add possibility to run study simulations #35

feat(api): add possibility to run study simulations #35

Conversation

salemsd commented Dec 12, 2024

MartinBelthle left a 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

MartinBelthle Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

MartinBelthle left a 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

MartinBelthle left a comment

Choose a reason for hiding this comment

MartinBelthle left a 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

MartinBelthle Dec 13, 2024 •

edited

Loading