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

Error handling #349

Merged
merged 10 commits into from
Jun 4, 2024
Merged

Error handling #349

merged 10 commits into from
Jun 4, 2024

Conversation

martinRenou
Copy link
Collaborator

@martinRenou martinRenou commented Apr 16, 2024

Fix #343

Screencast.from.2024-06-04.12-43-14.mp4

Copy link
Contributor

Binder 👈 Launch a Binder on branch martinRenou/jupytercad/error_handling

@martinRenou martinRenou added the enhancement New feature or request label Apr 16, 2024
@martinRenou martinRenou changed the title Draft: Error handling Error handling Jun 4, 2024
@martinRenou martinRenou marked this pull request as ready for review June 4, 2024 11:33
@trungleduc
Copy link
Member

Thanks @martinRenou for working on it. I just tested with some corner cases:

  • Adding shape or editing shape from the right panel does not go through the dry-run check, should we do it?
cad3.mp4
  • I artificially make the loadFlle action much slower so that I can add another operator while the first one is still processing. But then the first operator is discarded, we may need to lock the operators while the worker is busy
cad4.mp4
  • When there are 2 users, the first user adds an operator but since the shared doc is not updated, the second user might add another operator with the same name
cad2.mp4
  • Even if the second user can avoid the naming collision, the outcome is not correct since their worker does not know about the upcoming operator of the first user
cad5.mp4

I think we might need to improve the loading indicator and locking mechanism to avoid these cases

@martinRenou
Copy link
Collaborator Author

Adding shape or editing shape from the right panel does not go through the dry-run check, should we do it?

Yes, definitely. Also we should probably do it on the first load of the file, and report malformed a file if that's the case.

I artificially make the loadFlle action much slower so that I can add another operator while the first one is still processing. But then the first operator is discarded, we may need to lock the operators while the worker is busy

Right! We should fix it indeed. It's unrelated to this PR though, so we should probably turn this into an issue and look into implementing a fix.

When there are 2 users, the first user adds an operator but since the shared doc is not updated, the second user might add another operator with the same name

I believe name conflicts should not be a thing. We can fix it by fixing #200.

Even if the second user can avoid the naming collision, the outcome is not correct since their worker does not know about the upcoming operator of the first user

I guess this would be fixed by implementing a proper lock?

@trungleduc
Copy link
Member

trungleduc commented Jun 4, 2024

yeah, having a proper operator lock definitely helps. But I also feel that having a single promise delegate for all operators might be a bottleneck. Using a dict of promise delegates and sending the delegate id back and forth between the worker and the main view model can work? In the end we only need to resolve the correct promise from worker's response

@martinRenou
Copy link
Collaborator Author

Oh I see. I'll have a look at that.

Copy link
Member

@trungleduc trungleduc left a comment

Choose a reason for hiding this comment

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

Thanks 🚀

@martinRenou
Copy link
Collaborator Author

Wait before merging I'll check to use the dry run in the object properties panel too :)

@martinRenou
Copy link
Collaborator Author

@trungleduc Ready for review!

Co-authored-by: Duc Trung Le <leductrungxf@gmail.com>
@trungleduc
Copy link
Member

Thanks @martinRenou

@trungleduc trungleduc merged commit 5703a01 into jupytercad:main Jun 4, 2024
9 checks passed
@SylvainCorlay
Copy link
Member

🎉 that is pretty great!

@martinRenou martinRenou deleted the error_handling branch June 5, 2024 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement proper operator error handling
3 participants