Skip to content
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

Show cost for EME, HEAT, and MODE. Remove URL from EME webapi. #1634

Merged
merged 1 commit into from
May 9, 2024

Conversation

caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Apr 24, 2024

Tested for HEAT and EME.

@caseyflex caseyflex marked this pull request as draft April 24, 2024 09:34
@caseyflex caseyflex marked this pull request as ready for review April 24, 2024 09:36
@tylerflex tylerflex added the 2.7 will go into version 2.7.* label Apr 25, 2024
Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Thanks! I've merged your webapi PR and redeployed the service - give it a try now!

However, what exactly was happening before? In principle if the webapi errors, a metadata pipeline is meant to be kicked off such that the estimate should always eventually appear. Was that not what you were seeing?

My other comments are a bit nit-picky, but it would be great to avoid some code duplication.

How about we define a SOLVER_NAME dict that links task_type to the name we want printed in log messages. Then we can have a single message here and also replace this with just solver_name = SOLVER_NAME[task_type]?

tidy3d/web/api/webapi.py Outdated Show resolved Hide resolved
tidy3d/web/api/webapi.py Show resolved Hide resolved
@caseyflex caseyflex marked this pull request as draft April 30, 2024 15:34
@caseyflex
Copy link
Contributor Author

caseyflex commented Apr 30, 2024

Thanks! I've merged your webapi PR and redeployed the service - give it a try now!

However, what exactly was happening before? In principle if the webapi errors, a metadata pipeline is meant to be kicked off such that the estimate should always eventually appear. Was that not what you were seeing?

My other comments are a bit nit-picky, but it would be great to avoid some code duplication.

How about we define a SOLVER_NAME dict that links task_type to the name we want printed in log messages. Then we can have a single message here and also replace this with just solver_name = SOLVER_NAME[task_type]?

I see. When I called web.estimate_cost, I got an estimated cost of 0.0 and metadataStatus='success', even though the metadata pipeline is giving the correct estimated cost. I don't know if this is using the webapi or some other web service. This is actually still the case, so I'm not sure why the estimated cost is 0. It doesn't appear to be running the metadata pipeline in the estimate_cost function.

I see that now when I call web.run with verbose=True, it shows the correct behavior. estimated cost is 0 and metadataStatus is processing, then after a while estimated cost is correct and metadataStatus is processed. This must be because the metadataPipeline is working correctly.

I converted this to a draft, I need to test a bit more.

@caseyflex caseyflex marked this pull request as ready for review May 8, 2024 10:31
@caseyflex
Copy link
Contributor Author

@momchil-flex
I unified the treatment a bit between the solvers. It looked like the non-FDTD solver status loop was just a subset of the FDTD solver status loop. The unified logic seems to work well, and makes it easier to implement stuff like EME progress bar in the future. Could you check if this looks ok? If so, the PR should be good to merge.

@momchil-flex
Copy link
Collaborator

Looks good, played around a bit and it worked well. Hopefully no major issues will come up. :)

@momchil-flex momchil-flex merged commit 2f6f8c6 into pre/2.7 May 9, 2024
16 checks passed
@momchil-flex momchil-flex deleted the casey/emewebapi branch May 9, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.7 will go into version 2.7.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants