-
Notifications
You must be signed in to change notification settings - Fork 44
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
Adding estimate_cost to webapi #301
Conversation
Removing unnecessary _upload_task function (duplicate of upload) Log the estimated cost upon task upload Update the web tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a few comments about adding in some defensive error handling, especially for folder creation. See if that makes sense with the overall logical flow.
I also tested myself and it worked for me.
"taskName": task_name, | ||
"callbackUrl": callback_url, | ||
} | ||
folder = _query_or_create_folder(folder_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember seeing some errors related to this function call. do we want to wrap it in a try: except:? For example if the folder doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be good to know what errors out exactly? Also, what do we do if it errors - seems like the upload can't continue in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes to both, I wonder if this logic should just go in the query_or_create_folder
function
tidy3d/web/webapi.py
Outdated
raise WebError("Upload file to 3S failed, please check your network or try it later.") | ||
|
||
# log the url for the task in the web UI | ||
log.info(f"{DEFAULT_CONFIG.website_endpoint}/folders/{folder.projectId}/tasks/{task_id}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for me (jupyter notebook), this never displays great: I can't click the link nor can I copy and paste it. Maybe we can fix it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I'll have a quick look if it can be done better.
tidy3d/web/webapi.py
Outdated
else: | ||
data["protocolVersion"] = __version__ | ||
|
||
resp = http.post(method, data=data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to wrap this in try except too:
try:
resp = http.post(method=method, data=data)
except Exception as e:
log.warning("could not get flex unit information.")
return resp.get("flex_unit")
Does this fit with the expected use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All http calls are already wrapped in a handle_response
decorator in httputils.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said actually I think you're right, we may want to wrap this so that it doesn't error out if it doesn't get the cost for some reason. Technically if that happens chances are the simulation won't be able to run correctly anyway (something's wrong with the web settings?) But it may be useful for our develop purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea.. well it could just be something wrong with our estimator, or the flex unit was not included in the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it wasn't included in the response, it will just return None. Anyway the reason why I'm saying this should never happen is because the same metadata service is run when a job is started, and is required for the job to be picked up by the solver.
Removing unnecessary _upload_task function (duplicate of upload)
Log the estimated cost upon task upload
Update the web tests
This is my version of #300 . I had done most of the work yesterday but didn't push and then An made a different PR.