Simulation-related changes required for M4.5.7 #125

agarny · 2022-11-11T03:02:00Z

To be merged in ONLY IF datasets 135 and 157 have been published to the portal.

Description

Updated the SPARC API so that it can address the needs of M4.5.7. This meant removing the /simulation endpoint and replacing it with a new /start_simulation and /check_simulation endpoints. This was needed to account for simulation that take quite some time to run. The previous /simulation endpoint would try to start and check a simulation in one go, but if the simulation was too long then the API would time out. Now, we can start the simulation and check for its progress until it has completed or errored. The tests have been changed accordingly.

Also updated the /sim/dataset/<id_> endpoint to make it optional to output debug information (see inject_template_data()) which doesn't apply to simulation datasets. The default behaviour hasn't changed, but now if you have something like /sim/dataset/135/nodebug then we won't get that warning from inject_template_data().

Type of change

Delete those that don't apply.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added unit tests that prove my fix is effective or that my feature works

So that we can run both OpenCOR-related and oSPARC-related simulations.

With a /start_simulation and /check_simulation endpoint, so that we can start and check a longish simulation.

…imulation endpoints.

Needed for datasets 4 and 17.

Since we replaced our /simulation endpoint with the /start_simulation and /check_simulation endpoints.

Tehsurfer · 2022-12-12T00:47:45Z

@agarny is this PR still a draft?

I could use some help on how I should test the PR.

I tried running the osparc tests and passed all except:

test_osparc_successful_simulation

and

test_osparc_successful_simulation

agarny · 2022-12-12T01:35:10Z

I hadn't even noticed that it had been qualified as a draft, but no it's not a draft anymore. As for the failing tests, sorry about that, let me have a quick look.

agarny · 2022-12-12T01:44:08Z

@Tehsurfer, I have just run the SPARC API tests and they are all fine for me:

export PYTHONPATH=`pwd`
pip install -r requirements-dev.txt
pytest
.
.
.
=============================================== short test summary info ================================================
FAILED tests/test_api.py::test_map_get_share_id_and_state - assert 404 == 400
FAILED tests/test_api.py::test_scaffold_get_share_id_and_state - assert 404 == 400
FAILED tests/test_api.py::test_create_wrike_task - assert 400 == 200
FAILED tests/test_api.py::test_subscribe_to_mailchimp - TypeError: can only concatenate str (not "dict") to str
FAILED tests/test_dataset_info.py::test_generic_mouse_colon_dataset_search - assert 0 == 1
FAILED tests/test_dataset_info.py::test_complex_title_dataset_search - assert 0 == 1
FAILED tests/test_dataset_info.py::test_title_plot_annotation_dataset_search - assert 0 == 1
FAILED tests/test_dataset_info.py::test_object_identifier_dataset_search - assert 0 == 1
FAILED tests/test_dataset_info.py::test_pennsieve_identifier_dataset_search - assert 0 == 1
FAILED tests/test_scicrunch.py::test_scicrunch_dataset_doi - IndexError: list index out of range
FAILED tests/test_scicrunch.py::test_scicrunch_multiple_dataset_doi - IndexError: list index out of range
FAILED tests/test_scicrunch.py::test_scicrunch_multiple_dataset_ids - IndexError: list index out of range
FAILED tests/test_scicrunch.py::test_scicrunch_all_data - assert 30 > 40
FAILED tests/test_scicrunch.py::test_scicrunch_basic_search - assert 10 > 10
FAILED tests/test_scicrunch.py::test_response_version - IndexError: list index out of range
FAILED tests/test_scicrunch.py::test_response_abi_plot - IndexError: list index out of range
FAILED tests/test_scicrunch.py::test_response_abi_scaffold - IndexError: list index out of range
FAILED tests/test_scicrunch.py::test_response_sample_subject_size - assert 0 == 1
FAILED tests/test_scicrunch.py::test_scaffold_files - botocore.exceptions.ClientError: An error occurred (404) when c...
FAILED tests/test_scicrunch.py::test_finding_contextual_information - assert 0 > 0
FAILED tests/test_scicrunch.py::test_undefined_version_dataset_search - assert 0 == 1
=========================== 21 failed, 57 passed, 3 skipped, 4 warnings in 118.58s (0:01:58) ===========================

Ok, time to remind myself what those tests actually do and see why they pass for me and fail for you.

agarny · 2022-12-12T01:47:33Z

By the way, @Tehsurfer, you mentioned test test_osparc_successful_simulation twice...?

agarny · 2022-12-12T01:48:41Z

To run just the oSPARC tests:

$ pytest tests/test_osparc.py
================================================= test session starts ==================================================
platform darwin -- Python 3.9.15, pytest-5.4.3, py-1.10.0, pluggy-0.13.1
rootdir: /Users/Alan/Work/Programming/SPARCAPI
collected 14 items

tests/test_osparc.py ..............                                                                              [100%]

=================================================== warnings summary ===================================================
/Users/Alan/venv/SPARCAPI/lib/python3.9/site-packages/future/standard_library/__init__.py:65
  /Users/Alan/venv/SPARCAPI/lib/python3.9/site-packages/future/standard_library/__init__.py:65: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/latest/warnings.html
============================================ 14 passed, 1 warning in 32.22s ============================================

So, all good indeed...

agarny · 2022-12-12T01:53:12Z

@Tehsurfer, what kind of error message are you getting? FWIW, I have just modified test_osparc_successful_simulation to make it fail and that's exactly what happened when rerunning the test:

$ pytest tests/test_osparc.py
================================================= test session starts ==================================================
platform darwin -- Python 3.9.15, pytest-5.4.3, py-1.10.0, pluggy-0.13.1
rootdir: /Users/Alan/Work/Programming/SPARCAPI
collected 14 items

tests/test_osparc.py ............F.                                                                              [100%]

======================================================= FAILURES =======================================================
__________________________________________ test_osparc_successful_simulation ___________________________________________

client = <FlaskClient <Flask 'app.main'>>

    def test_osparc_successful_simulation(client):
        data = {
            "opencor": {
                "model_url": "https://models.physiomeproject.org/e/611/HumanSAN_Fabbri_Fantini_Wilders_Severi_2017.cellml",
                "json_config": {
                    "simulation": {
                        "Ending point": 0.003,
                        "Point interval": 0.001,
                    },
                    "output": ["Membrane/V"]
                }
            },
            "solver": {
                "name": "simcore/services/comp/opencor",
                "version": "1.0.3"
            }
        }
        res = {
            "status": "ok",
            "results": {
                "environment/time": [0.0, 0.1, 0.002, 0.003],
                "Membrane/V": [-47.787168, -47.74547155339473, -47.72515226841376, -47.71370033208329]
            }
        }
        r = client.post("/start_simulation", json=data)
        assert r.status_code == 200
        check_simulation_data = json.loads(r.data)["data"]
        while True:
            r = client.post("/check_simulation", json=check_simulation_data)
            assert r.status_code == 200
            json_data = json.loads(r.data)
            assert json_data["status"] == "ok"
            if "results" in json_data:
>               assert json.dumps(json_data, sort_keys=True) == json.dumps(res, sort_keys=True)
E               assert '{"results": ...tatus": "ok"}' == '{"results": ...tatus": "ok"}'
E                 Skipping 119 identical leading characters in diff, use -v to show
E                 - : [0.0, 0.1, 0.002, 0.003]}, "status": "ok"}
E                 + : [0.0, 0.001, 0.002, 0.003]}, "status": "ok"}
E                 ?           ++

tests/test_osparc.py:142: AssertionError
=================================================== warnings summary ===================================================
/Users/Alan/venv/SPARCAPI/lib/python3.9/site-packages/future/standard_library/__init__.py:65
  /Users/Alan/venv/SPARCAPI/lib/python3.9/site-packages/future/standard_library/__init__.py:65: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    import imp

-- Docs: https://docs.pytest.org/en/latest/warnings.html
=============================================== short test summary info ================================================
FAILED tests/test_osparc.py::test_osparc_successful_simulation - assert '{"results": ...tatus": "ok"}' == '{"results"...
======================================= 1 failed, 13 passed, 1 warning in 29.90s =======================================

So, yes, I really can't see what might be wrong for you.

This test relies on a file in dataset 135 that has changed.

agarny · 2022-12-12T21:54:58Z

@alan-wu and @Tehsurfer, I have just updated this PR to be in sync with the master branch and also fixed a small issue with test_simulation_ui_file in our API tests since it relies on a file in dataset 135 that has been updated.

Tehsurfer · 2022-12-13T00:42:33Z

@agarny yeah I get the error:

with open(n, 'rb') as f:
E                   PermissionError: [Errno 13] Permission denied: 'C:\\Users\\jkho021\\AppData\\Local\\Temp\\tmp21eblmpe'

Which my guess is comes from this line?

temp_input_file = tempfile.NamedTemporaryFile(mode="w+")

not to sure

agarny · 2022-12-13T00:49:43Z

Still a Windows-related issue to me, not to mention that it's already used in v1.9.0 of the SPARC API (see here).

Tehsurfer · 2022-12-13T00:51:28Z

Ok I'll try testing by sending requests from here:

nih-sparc/sparc-app#568

Tehsurfer · 2022-12-13T01:10:43Z

@agarny Yeah I managed to fix that permission error by changing

temp_config_file = tempfile.NamedTemporaryFile(mode="w+")
to
temp_config_file = tempfile.NamedTemporaryFile(mode="w+", delete=False)

But now I am getting this error:

        assert r.status_code == 200
>       check_simulation_data = json.loads(r.data)["data"]
E       KeyError: 'data'

agarny · 2022-12-13T01:26:52Z

I am not sure we want delete=False. Temporary files should be just that: temporary, which means that they should be deleted once not needed anymore. But, maybe it was just for you to test things?

As for the data key issue, I am not sure why it would complain about it since it works fine for me...!?

Tehsurfer · 2022-12-13T01:38:01Z

Ok I figured it out, needed Osparc keys and the tempfile.NamedTemporaryFile(mode="w+", delete=False) and now working

found it from here: https://stackoverflow.com/questions/23212435/permission-denied-to-write-to-my-temporary-file

Tehsurfer

Worked for me!

Mostly looked pretty good but I found the if statements a bit hard to follow at times

Tehsurfer · 2022-12-13T01:47:42Z

app/main.py

@@ -685,13 +688,14 @@ def build_filetypes_table(osparc_viewers):


 @app.route("/sim/dataset/<id_>")
-def sim_dataset(id_):
+@app.route("/sim/dataset/<id_>/<debug_>")


Do you think using DEPLOY_ENV would be better than having a route for debugging?

ie:

except ClientError: # If the file is not under folder 'files', check under folder 'packages' if Config.DEPLOY_ENV is not "production" logging.warning( "Required file template.json was not found under /files folder, trying under /packages..." )

I don't like having a route for debugging, BUT that warning (and also error further down the code) is only relevant for SIM-Core based simulation datasets (depending on what you do with the /sim/dataset/<id_> endpoint), not CellML-based simulation datasets.

Here, I am trying to reuse the /sim/dataset/<id_> endpoint and that warning (and error) is not relevant to me, no matter whether it's a SIM-Core based simulation or a CellML-based one. So, I just don't want to see that message at all, be it in production or in my development environment. It clutters my console for no good reason.

I could have copied/pasted most of that code, but that would have been a bit silly, hence the debugging route approach.

A debug flag could be setup using a environment variable instead. The debug route is not useful to the person calling it unless they are also running the server.

A debug flag could be setup using a environment variable instead. The debug route is not useful to the person calling it unless they are also running the server.

Sure, although such an environment variable would normally be set IF we want to get debug information while, here, debug information is provided by default (which is wrong in the first place).

Anyway, what should such an environment variable be called? What about SPARC_API_DEBUGGING? If it's not set or set to 1 (or ON?) then debugging would be on, and if it is set to 0 (or OFF?) then debugging would be off.

SPARC_API_DEBUGGING is good. Having it default to be on is consistent with the DEPLOY_ENV = development so that is fine. TRUE or FALSE is probably more consistent with standard although it is going to be string comparison regardless.

SPARC_API_DEBUGGING is good. Having it default to be on is consistent with the DEPLOY_ENV = development so that is fine. TRUE or FALSE is probably more consistent with standard although it is going to be string comparison regardless.

Ok, going to use SPARC_API_DEBUGGING with unset meaning TRUE.

Tehsurfer · 2022-12-13T02:35:07Z

app/osparc.py

 from time import sleep


+OPENCOR_SOLVER = "simcore/services/comp/opencor"
+DATASET_4_SOLVER = "simcore/services/comp/rabbit-ss-0d-cardiac-model"
+DATASET_17_SOLVER = "simcore/services/comp/human-gb-0d-cardiac-model"


Would switching to a dictionary remove some of the if statement?

Something like:

SOLVER_LOOKUP = {
"OPENCOR_SOLVER": "simcore/services/comp/opencor",
"DATASET_4_SOLVER": "simcore/services/comp/rabbit-ss-0d-cardiac-model",

...

}

I can't see how it would. Here, we really want to distinguish between CellML-based simulation datasets (which require OPENCOR_SOLVER to be solved) and SIM-Core based simulation datasets (which require another solver). In the latter case, to have DATASET_4_SOLVER, DATASET_17_SOLVER, and DATASET_78_SOLVER is just so that we can provide a more accurate user feedback, should we ever try to run a simulation using a solver that is not supported.

Tehsurfer · 2022-12-13T02:35:55Z

app/osparc.py

+            password=Config.OSPARC_API_SECRET
+        ))
+        solvers_api = osparc.SolversApi(api_client)
+        job_id = data["job_id"]


Do we need to check the keys from data? Or are they always correct?

data comes from oSPARC and, unless they change their JSON schema (which would break many things for them, I would imagine), we should be fine. Also, this is the reason we do all of that stuff in a try...except statement. If something goes wrong, we report an error.

The data is checked on line 1145 in main.py.

The data is checked on line 1145 in main.py.

Oh yes, I had forgotten that I had done that check. :)

Thanks @alan-wu! I missed that

Tehsurfer · 2022-12-13T02:37:07Z

app/osparc.py

-def run_simulation(model_url, json_config):
-    with tempfile.NamedTemporaryFile(mode="w+") as temp_config_file:
-        json.dump(json_config, temp_config_file)
+def start_simulation(data):


Is it worth splitting up start simulation into a few more functions? The flow is kind of hard for me to follow with all of the if statements

I tried to think of a way to split this function but couldn't come up with a simple solution, not least since it relies on the use of exceptions which would need to be propagated.

…mation.

alan-wu

Looks good.

agarny added 16 commits August 24, 2022 15:25

Tests: some minor cleaning up.

c8680fc

/sim/dataset: added an optional debug parameter.

caa7150

Some minor cleaning up.

53e53cb

Updated the /simulation endpoint.

a8a4970

So that we can run both OpenCOR-related and oSPARC-related simulations.

Merge branch 'master' into simulation.

04d926d

Don't support the 1D and 2D solvers for datasets 4 and 17.

300d00f

Some minor cleaning up.

333f502

Some minor refactoring.

0ee08b4

Renamed the /simulation endpoint to /start_simulation.

ee06716

Some minor cleaning up.

7cbffc3

Replaced the /simulation endpoint.

2ac972f

With a /start_simulation and /check_simulation endpoint, so that we can start and check a longish simulation.

Changed a bit what is expected for the /start_simulation and /check_s…

84f25d2

…imulation endpoints.

Upload a solver's input file, if present.

7219d1a

Needed for datasets 4 and 17.

Tests: updated the oSPARC-related tests.

ff85f35

Since we replaced our /simulation endpoint with the /start_simulation and /check_simulation endpoints.

Merge branch 'master' into simulation.

511c951

Merge branch 'master' into simulation.

3b49afd

nickerso marked this pull request as draft November 13, 2022 10:36

agarny marked this pull request as ready for review December 12, 2022 01:35

agarny added 2 commits December 13, 2022 10:40

Merge branch 'master' into simulation.

132ac98

API tests: fixed test_simulation_ui_file.

3219e04

This test relies on a file in dataset 135 that has changed.

Tehsurfer approved these changes Dec 13, 2022

View reviewed changes

agarny force-pushed the simulation branch 2 times, most recently from 94fb198 to c7fa94a Compare December 14, 2022 00:33

Use an environment variable to decide whether to show debugging infor…

da8ca4d

…mation.

agarny force-pushed the simulation branch from c7fa94a to da8ca4d Compare December 14, 2022 00:58

Merge branch 'master' into simulation

dbc99db

alan-wu approved these changes Dec 15, 2022

View reviewed changes

alan-wu approved these changes Dec 18, 2022

View reviewed changes

alan-wu merged commit 0ceeadb into nih-sparc:master Dec 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simulation-related changes required for M4.5.7 #125

Simulation-related changes required for M4.5.7 #125

agarny commented Nov 11, 2022 •

edited

Loading

Tehsurfer commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

Tehsurfer commented Dec 13, 2022

agarny commented Dec 13, 2022

Tehsurfer commented Dec 13, 2022

Tehsurfer commented Dec 13, 2022

agarny commented Dec 13, 2022

Tehsurfer commented Dec 13, 2022 •

edited

Loading

Tehsurfer left a comment

Tehsurfer Dec 13, 2022

agarny Dec 13, 2022

alan-wu Dec 13, 2022 •

edited

Loading

agarny Dec 13, 2022

alan-wu Dec 13, 2022

agarny Dec 14, 2022

Tehsurfer Dec 13, 2022

agarny Dec 13, 2022

Tehsurfer Dec 13, 2022

agarny Dec 13, 2022

alan-wu Dec 13, 2022 •

edited

Loading

agarny Dec 13, 2022

Tehsurfer Dec 14, 2022

Tehsurfer Dec 13, 2022

agarny Dec 13, 2022

alan-wu left a comment

Simulation-related changes required for M4.5.7 #125

Simulation-related changes required for M4.5.7 #125

Conversation

agarny commented Nov 11, 2022 • edited Loading

To be merged in ONLY IF datasets 135 and 157 have been published to the portal.

Description

Type of change

How Has This Been Tested?

Checklist:

Tehsurfer commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

agarny commented Dec 12, 2022

Tehsurfer commented Dec 13, 2022

agarny commented Dec 13, 2022

Tehsurfer commented Dec 13, 2022

Tehsurfer commented Dec 13, 2022

agarny commented Dec 13, 2022

Tehsurfer commented Dec 13, 2022 • edited Loading

Tehsurfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-wu Dec 13, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alan-wu Dec 13, 2022 • 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

alan-wu left a comment

Choose a reason for hiding this comment

agarny commented Nov 11, 2022 •

edited

Loading

Tehsurfer commented Dec 13, 2022 •

edited

Loading

alan-wu Dec 13, 2022 •

edited

Loading

alan-wu Dec 13, 2022 •

edited

Loading