Skip to content

Simulation-related changes required for M4.5.7 #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Dec 18, 2022
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 32 additions & 18 deletions app/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
reform_related_terms
from app.serializer import ContactRequestSchema
from app.utilities import img_to_base64_str
from app.osparc import run_simulation
from app.osparc import start_simulation as do_start_simulation
from app.osparc import check_simulation as do_check_simulation
from app.biolucida_process_results import process_results as process_biolucida_results

logging.basicConfig()
Expand Down Expand Up @@ -171,14 +172,14 @@ def get_metrics():
if contentful:
cf_response = get_funded_projects_count(contentful)
usage_metrics['funded_projects_count'] = cf_response

ps_response = get_download_count()
usage_metrics['1year_download_count'] = ps_response

if not metrics_scheduler.running:
logging.info('Starting scheduler for metrics acquisition')
metrics_scheduler.start()


# Gets oSPARC viewers before the first request after startup and then once a day.
viewers_scheduler.add_job(func=get_osparc_file_viewers, trigger="interval", days=1)
Expand Down Expand Up @@ -629,7 +630,7 @@ def inject_markdown(resp):
resp["markdown"] = mark_req.text


def inject_template_data(resp):
def inject_template_data(resp, debug):
id_ = resp.get("id")
version = resp.get("version")
if id_ is None or version is None:
Expand All @@ -643,17 +644,19 @@ def inject_template_data(resp):
)
except ClientError:
# If the file is not under folder 'files', check under folder 'packages'
logging.warning(
"Required file template.json was not found under /files folder, trying under /packages..."
)
if debug:
logging.warning(
"Required file template.json was not found under /files folder, trying under /packages..."
)
try:
response = s3.get_object(
Bucket="pennsieve-prod-discover-publish-use1",
Key="{}/{}/packages/template.json".format(id_, version),
RequestPayer="requester",
)
except ClientError as e:
logging.error(e)
if debug:
logging.error(e)
return

template = response["Body"].read()
Expand Down Expand Up @@ -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_>")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..."
            )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@alan-wu alan-wu Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def sim_dataset(id_, debug_="debug"):
if request.method == "GET":
req = requests.get("{}/datasets/{}".format(Config.DISCOVER_API_HOST, id_))
if req.ok:
json_data = req.json()
inject_markdown(json_data)
inject_template_data(json_data)
inject_template_data(json_data, debug_ == "debug")
return jsonify(json_data)
abort(404, description="Resource not found")

Expand Down Expand Up @@ -959,7 +963,7 @@ def create_wrike_task():
headers=headers
)
except Exception as e:
print(e)
print(e)

if (resp.status_code == 200):
if 'userEmail' in form and form['userEmail'] is not None:
Expand Down Expand Up @@ -1084,10 +1088,10 @@ def get_available_uberonids():
return jsonify(result)


# Get list of terms a level up/down from
# Get list of terms a level up/down from
@app.route("/get-related-terms/<query>")
def get_related_terms(query):

payload = {
'direction': request.args.get('direction', default='OUTGOING'),
'relationshipType': request.args.get('relationshipType', default='BFO:0000050'),
Expand Down Expand Up @@ -1124,14 +1128,24 @@ def simulation_ui_file(identifier):
abort(404, description="no simulation UI file could be found")


@app.route("/simulation", methods=["POST"])
def simulation():
@app.route("/start_simulation", methods=["POST"])
def start_simulation():
data = request.get_json()

if data and "solver" in data and "name" in data["solver"] and "version" in data["solver"]:
return json.dumps(do_start_simulation(data))
else:
abort(400, description="Missing solver name and/or solver version")


@app.route("/check_simulation", methods=["POST"])
def check_simulation():
data = request.get_json()

if data and "model_url" in data and "json_config" in data:
return json.dumps(run_simulation(data["model_url"], data["json_config"]))
if data and "job_id" in data and "solver" in data and "name" in data["solver"] and "version" in data["solver"]:
return json.dumps(do_check_simulation(data))
else:
abort(400, description="Missing model URL and/or JSON configuration")
abort(400, description="Missing solver name, solver version and/or job id")


@app.route("/pmr_latest_exposure", methods=["POST"])
Expand Down
207 changes: 153 additions & 54 deletions app/osparc.py
Original file line number Diff line number Diff line change
@@ -1,71 +1,159 @@
from app.config import Config
import json
import osparc
import tempfile

from app.config import Config
from flask import abort
from osparc.rest import ApiException
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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",

...

}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DATASET_78_SOLVER = "simcore/services/comp/kember-cardiac-model"


class SimulationException(Exception):
pass


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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# Determine the type of simulation.

temp_config_file.seek(0)
solver_name = data["solver"]["name"]

try:
api_client = osparc.ApiClient(osparc.Configuration(
host=Config.OSPARC_API_URL,
username=Config.OSPARC_API_KEY,
password=Config.OSPARC_API_SECRET
))
if solver_name == OPENCOR_SOLVER:
if not "opencor" in data:
abort(400, description="Missing OpenCOR settings")
else:
if "osparc" in data:
if ((solver_name != DATASET_4_SOLVER)
and (solver_name != DATASET_17_SOLVER)
and (solver_name != DATASET_78_SOLVER)):
abort(400, description="Unknown oSPARC solver")
else:
abort(400, description="Missing oSPARC settings")

# Upload the configuration file.
# Start the simulation.

files_api = osparc.FilesApi(api_client)
try:
api_client = osparc.ApiClient(osparc.Configuration(
host=Config.OSPARC_API_URL,
username=Config.OSPARC_API_KEY,
password=Config.OSPARC_API_SECRET
))

try:
config_file = files_api.upload_file(temp_config_file.name)
except:
raise SimulationException(
"the simulation configuration file could not be uploaded")
# Upload the configuration file, in the case of an OpenCOR simulation or
# in the case of an oSPARC simulation input file.

# Create the simulation.
has_solver_input = "input" in data["solver"]

solvers_api = osparc.SolversApi(api_client)
if solver_name == OPENCOR_SOLVER:
temp_config_file = tempfile.NamedTemporaryFile(mode="w+")

solver = solvers_api.get_solver_release(
"simcore/services/comp/opencor", "1.0.3")
json.dump(data["opencor"]["json_config"], temp_config_file)

temp_config_file.seek(0)

try:
files_api = osparc.FilesApi(api_client)

job = solvers_api.create_job(
solver.id,
solver.version,
osparc.JobInputs({
"model_url": model_url,
"config_file": config_file
})
)
config_file = files_api.upload_file(temp_config_file.name)
except ApiException as e:
raise SimulationException(
f"the simulation configuration file could not be uploaded ({e})")

# Start the simulation job.
temp_config_file.close()
elif has_solver_input:
temp_input_file = tempfile.NamedTemporaryFile(mode="w+")

status = solvers_api.start_job(solver.id, solver.version, job.id)
temp_input_file.write(data["solver"]["input"]["value"])
temp_input_file.seek(0)

if status.state != "PUBLISHED":
raise SimulationException("the simulation job could not be submitted")
try:
files_api = osparc.FilesApi(api_client)

# Wait for the simulation job to be complete (or to fail).
input_file = files_api.upload_file(temp_input_file.name)
except ApiException as e:
raise SimulationException(
f"the solver input file could not be uploaded ({e})")

while True:
status = solvers_api.inspect_job(solver.id, solver.version, job.id)
temp_input_file.close()

if status.progress == 100:
break
# Create the simulation job with the job inputs that matches our
# simulation type.

sleep(1)
solvers_api = osparc.SolversApi(api_client)

status = solvers_api.inspect_job(solver.id, solver.version, job.id)
try:
solver = solvers_api.get_solver_release(
solver_name, data["solver"]["version"])
except ApiException as e:
raise SimulationException(
f"the requested solver could not be retrieved ({e})")

if solver_name == OPENCOR_SOLVER:
job_inputs = {
"model_url": data["opencor"]["model_url"],
"config_file": config_file
}
else:
if has_solver_input:
data["osparc"]["job_inputs"][data["solver"]["input"]["name"]] = input_file

job_inputs = data["osparc"]["job_inputs"]

job = solvers_api.create_job(
solver.id,
solver.version,
osparc.JobInputs(job_inputs)
)

# Start the simulation job.

status = solvers_api.start_job(solver.id, solver.version, job.id)

if status.state != "PUBLISHED":
raise SimulationException(
"the simulation job could not be submitted")

res = {
"status": "ok",
"data": {
"job_id": job.id,
"solver": {
"name": solver.id,
"version": solver.version
}
}
}
except SimulationException as e:
res = {
"status": "nok",
"description": e.args[0] if len(e.args) > 0 else "unknown"
}

return res


def check_simulation(data):
try:
# Check whether the simulation has completed (or failed).

api_client = osparc.ApiClient(osparc.Configuration(
host=Config.OSPARC_API_URL,
username=Config.OSPARC_API_KEY,
password=Config.OSPARC_API_SECRET
))
solvers_api = osparc.SolversApi(api_client)
job_id = data["job_id"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@alan-wu alan-wu Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alan-wu! I missed that

solver_name = data["solver"]["name"]
solver_version = data["solver"]["version"]
status = solvers_api.inspect_job(solver_name, solver_version, job_id)

if status.progress == 100:
# The simulation has completed, but was it successful?

if status.state != "SUCCESS":
raise SimulationException("the simulation failed")
Expand All @@ -74,33 +162,44 @@ def run_simulation(model_url, json_config):

try:
outputs = solvers_api.get_job_outputs(
solver.id, solver.version, job.id)
except:
solver_name, solver_version, job_id)
except ApiException as e:
raise SimulationException(
"the simulation job outputs could not be retrieved")
f"the simulation job outputs could not be retrieved ({e})")

# Download the simulation results.

try:
files_api = osparc.FilesApi(api_client)

results_filename = files_api.download_file(
outputs.results["output_1"].id)
except:
raise SimulationException("the simulation results could not be retrieved")
outputs.results[list(outputs.results.keys())[0]].id)
except ApiException as e:
raise SimulationException(
f"the simulation results could not be retrieved ({e})")

results_file = open(results_filename, "r")

res = {
"status": "ok",
"results": json.load(results_file)
}

if solver_name == OPENCOR_SOLVER:
res["results"] = json.load(results_file)
else:
res["results"] = results_file.read()

results_file.close()
except SimulationException as e:
else:
# The simulation is not complete yet.

res = {
"status": "nok",
"description": e.args[0] if len(e.args) > 0 else "unknown"
"status": "ok"
}
except SimulationException as e:
res = {
"status": "nok",
"description": e.args[0] if len(e.args) > 0 else "unknown"
}

temp_config_file.close()

return res
return res
Loading