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

Metadata visualization improvement #55

Merged
merged 40 commits into from
Apr 17, 2024

Conversation

garrettmflynn
Copy link
Collaborator

fix #54

@garrettmflynn garrettmflynn self-assigned this Apr 8, 2024
@garrettmflynn garrettmflynn changed the base branch from main to doc_improvements April 8, 2024 20:38
@CodyCBakerPhD
Copy link
Member

OOooo, something else I forgot to requestt - would it be possible to add a 'cancel' button on the frontend instead of having to ctrl+C the server each time? (could be useful in the GUIDE as well to cancel a conversion/inspection/upload operation to the best of our ability

Base automatically changed from doc_improvements to main April 10, 2024 18:57
@CodyCBakerPhD
Copy link
Member

@garrettmflynn Reminder on conflicts here - let me know when you removed the websocket stuff too and I'll take a look at this

@garrettmflynn
Copy link
Collaborator Author

Good to go! Haven't gotten around to the cancel request yet—but I'll see what I can do.

@garrettmflynn
Copy link
Collaborator Author

OOooo, something else I forgot to requestt - would it be possible to add a 'cancel' button on the frontend instead of having to ctrl+C the server each time? (could be useful in the GUIDE as well to cancel a conversion/inspection/upload operation to the best of our ability

Hmm this is somewhat complicated given that HTTP requests are one-off. I've seen a few hacks online that indicate that you can detect if the client aborts the request while it's occuring—but these are not super clean.

The only other way to do this would be to maintain global references to the running bars / processes and have a /cancel endpoint that can clean these up.

Are these solutions something that you would be interested in?

@CodyCBakerPhD
Copy link
Member

There's no way to track the PIDs of the processes and send a pkill or similar signal? I know how that might be done on the Python side (so through an endpoint) if JS doesn't have a way - in that case you'd have a 'cancel' button that would call the endpoints which would trigger the process kill?

@garrettmflynn
Copy link
Collaborator Author

Using that approach, we could notify the browser of related pids using SSE events as the processes are created. Though AFAIK this wouldn't abort the endpoint execution itself.

Would that actually achieve what you want? And I'm assuming you really only care about canceling in the parallel demo?

Otherwise, we could probably execute everything in this endpoint as a thread, then use the hacks referenced before to send some kill command to that—which would have references to everything directly and could shut it down that way.

@CodyCBakerPhD
Copy link
Member

OK after digging deeper into cancellation, the only/best 'safe' way to do this would be to have it as a specific feature of the iteration process on the backend

I have an idea for how to implement this in a follow up so I'll kick the bucket down the chain for now

@garrettmflynn
Copy link
Collaborator Author

Done!

Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.00%. Comparing base (68e1159) to head (281f01a).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #55      +/-   ##
==========================================
+ Coverage   50.00%   51.00%   +1.00%     
==========================================
  Files           6        6              
  Lines         100      100              
==========================================
+ Hits           50       51       +1     
+ Misses         50       49       -1     
Flag Coverage Δ
unittests 51.00% <100.00%> (+1.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...m_publisher/_demos/_demo_command_line_interface.py 0.00% <ø> (ø)
src/tqdm_publisher/_publisher.py 100.00% <100.00%> (ø)

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) April 17, 2024 04:51
@CodyCBakerPhD CodyCBakerPhD merged commit 4d2763b into main Apr 17, 2024
13 of 14 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the metadata-visualization-improvement branch April 17, 2024 04:52
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.

Next steps for improvements of all demo progress bars
2 participants