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

% Done Monitoring #241

Merged
merged 9 commits into from
Mar 16, 2022
Merged

% Done Monitoring #241

merged 9 commits into from
Mar 16, 2022

Conversation

tylerflex
Copy link
Collaborator

So this is looking pretty good on the front end, but needs some testing and a bit of refinement.

How it works now:

  1. Use console.status (running man animation) to loop through pre-processing statuses.
  2. Log that the solver is starting until we can successfully grab the % done data.
  3. Progressbar through % done and print field_Decay in progress bar description.
  4. Use console.status (running man animation) to loop through post-processing statuses

Simplifies a lot.
Job uses web.monitor() now.

Remaining issues:

  • Takes a long time to start getting % done, by which point the task is already completed. Need to either reduce this amount of time or change the message.
  • Needs some more testing by us before I feel confident releasing it.

@tylerflex tylerflex requested a review from momchil-flex March 3, 2022 22:28
@tylerflex tylerflex mentioned this pull request Mar 3, 2022
3 tasks
@tylerflex
Copy link
Collaborator Author

issue #140

@tylerflex tylerflex changed the title Tyler/%done % Done Monitoring Mar 3, 2022
@momchil-flex
Copy link
Collaborator

This is pretty decent for tasks that take longer than a minute. I'm down to merge - is there any specific testing you have in mind? Let's check in with Lei again about making the solver_progress.csv available earlier than after 100s.

Just one thing maybe to fix, this ran to 32% before early shutoff, but then it shows 100% done. Should we fix that?

image

@tylerflex
Copy link
Collaborator Author

Yea I think it marks the progress bar as "complete" when it's done running. I dont know if it would be more confusing to leave like this or to change it to end at 32% (potentially making people think the job didn't finish). What do you think?

@tylerflex
Copy link
Collaborator Author

No specific testing in mind, just wanted to get another pair of eyes on it.

@momchil-flex
Copy link
Collaborator

Yea I think it marks the progress bar as "complete" when it's done running. I dont know if it would be more confusing to leave like this or to change it to end at 32% (potentially making people think the job didn't finish). What do you think?

I feel like given that this is the full output, it won't be confusing, and it would provide the extra information about at what point the shutoff happened.

image

@tylerflex
Copy link
Collaborator Author

Ok I pushed this fix. If it's no longer running but the perc_done < 100 it will just display the message about early shutoff and leave the progress bar where it was. If it's no longer running but perc_done >= 100 it will explicitly set the progress bar to "completed" like you see in all cases before this change.

@momchil-flex
Copy link
Collaborator

Alright Lei pushed the change to get solver_progress in the first 10s. I tried a simulation that took 33s total and it worked pretty well! OK to merge as far as I'm concerned.

@momchil-flex
Copy link
Collaborator

Oh, sorry, it does seem to print "success" twice

image

@momchil-flex
Copy link
Collaborator

momchil-flex commented Mar 15, 2022

Also, in old tidy3d, in the monitor function, we do something like status = "success" if status == "visualize" and we don't print "visualize" as a status. This is because this step is the exporting of images (the three cross sections so far) on the server side, but the simulation data is already available to the user before this step is finished.

@tylerflex
Copy link
Collaborator Author

ok my last commit should do it, I refactored out the get_status() operation and made it return success if visualize. Then, I fixed a minor bug where the shutoff message wasn't getting displayed in a weird edge case. I also checked on a few notebooks that this works as expected.

@tylerflex tylerflex merged commit 62b3478 into develop Mar 16, 2022
@momchil-flex momchil-flex deleted the tyler/%done branch March 17, 2022 18:08
daquinteroflex pushed a commit to daquinteroflex/tidy3d that referenced this pull request Nov 7, 2023
…photonic_crystals

Alec/photonic crystals
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants