Skip to content

Conversation

KIRA009
Copy link
Contributor

@KIRA009 KIRA009 commented Jul 16, 2024

What kind of change does this PR introduce?
This PR addresses the root cause of #841

Summary

This PR runs all multiprocessing.Processtargets inside a try catch block, so any errors caused from within the target are gracefully handled.

The second part of this PR modifies the logger used by all processes to replace the default handler with a file handler, so all logs (including any exception traces) are written to the log file and can be used for debugging.

Checklist

  • My code follows the style guidelines of OpenAdapt
  • I have performed a self-review of my code
  • If applicable, I have added tests to prove my fix is functional/effective
  • I have linted my code locally prior to submission
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
  • New and existing unit tests pass locally with my changes

How can your code be run and tested?
Introduce an exception in one of the crud methods (e.g. insert_window_event) and then build the app (python -m openadapt.build) and run the built app. Start and complete a recording. The app should not crash and the logs should contain the exception raised in the crud method. The logs will be available at the ~/Library/Preferences/openadapt/data in Macos and AppData/Roaming/openadapt in Windows

@KIRA009
Copy link
Contributor Author

KIRA009 commented Jul 16, 2024

The second commit is entirely about replacing uses of the loguru logger with the custom logger, so the only commit that has functional changes is the first one - 4307a65

@abrichr
Copy link
Member

abrichr commented Jul 16, 2024

Thank you @KIRA009 ! Have you tested this on the built application and confirmed it's working as intended? Can you please show some screenshots confirming this to be the case?

@KIRA009
Copy link
Contributor Author

KIRA009 commented Jul 17, 2024

@abrichr Yes, I have tested it on mac. My windows machine is under repair so I am not able to test on that for the moment.

newmovie.mov

@abrichr
Copy link
Member

abrichr commented Jul 17, 2024

Thank you. What do you think about adding a test to assert that the application log is created and is non-empty?

Also, why does it say "Window events are not yet supported"?

@KIRA009
Copy link
Contributor Author

KIRA009 commented Jul 17, 2024

@abrichr That is a custom exception that I raised just to simulate an exception. The exception message is not real and not related to the app

@abrichr abrichr merged commit 13bbe48 into OpenAdaptAI:main Jul 19, 2024
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