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

Refactor config and pyinstaller build #618

Merged
merged 8 commits into from
May 1, 2024

Conversation

KIRA009
Copy link
Contributor

@KIRA009 KIRA009 commented Apr 13, 2024

What kind of change does this PR introduce?
This PR changes the config to use json files, and exposes some configuration for modifications through the dashboard. This PR also adds build scripts to create executables for both macos and windows os-es.

Screen.Recording.2024-04-20.at.10.58.44.PM.mov

Summary

This PR adds a page to the dashboard that will allow users to alter certain configuration settings for the project.

It also modifies the release workflow to add a build step that builds the executables for windows and macos, and then uploads them as artifacts to the latest release

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?
To build the project into an executable app, run the following command in a poetry shell

python -m openadapt.build

To run the project, you can now use the following command in a poetry shell

python -m openadapt.entrypoint

@KIRA009 KIRA009 force-pushed the feature/refactor-config branch 18 times, most recently from 508b7b8 to c4e36c5 Compare April 18, 2024 15:02
@KIRA009 KIRA009 force-pushed the feature/refactor-config branch from 6413723 to 6d95768 Compare April 20, 2024 17:07
@KIRA009 KIRA009 marked this pull request as ready for review April 20, 2024 17:38
@KIRA009 KIRA009 force-pushed the feature/refactor-config branch 2 times, most recently from ca04a15 to f269ecf Compare April 20, 2024 18:18
@KIRA009
Copy link
Contributor Author

KIRA009 commented Apr 20, 2024

@abrichr This should be ready for review now, I have rebased all the functional changes to a separate commit - ded9c66, and the subsequent commit only contains changes of updating the references of the config object in various files

Copy link
Member

@abrichr abrichr left a comment

Choose a reason for hiding this comment

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

Excellent work! I left a few comments, please let me know if you would like to chat about any of it 🙏

uses: actions/checkout@v4
with:
fetch-depth: 0
- name: Set up Pythonx`
Copy link
Member

Choose a reason for hiding this comment

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

This appears to be a typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

.github/workflows/release-and-publish.yml Show resolved Hide resolved
.github/workflows/release-and-publish.yml Show resolved Hide resolved
build_scripts/macos.sh Show resolved Hide resolved
openadapt/alembic/context_loader.py Show resolved Hide resolved


from openadapt.alembic.context_loader import load_alembic_context
from openadapt.app import tray # noqa
Copy link
Member

Choose a reason for hiding this comment

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

Why is noqa required here?

openadapt/privacy/providers/presidio.py Show resolved Hide resolved
@@ -967,11 +972,15 @@ def read_mouse_events(
@trace(logger)
def record(
task_description: str,
terminate_event: multiprocessing.Event = None,
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about renaming this terminate_process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While it makes sense, this flag is mostly used as a way to stop from reading and processing events, so maybe terminate_event_processing?

Copy link
Member

Choose a reason for hiding this comment

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

Or terminate_processing to be more terse, either works

sys.stdout = self.old_stdout
sys.stderr = self.old_stderr

return StdoutStderrOverride()
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about this slight refactor:

def is_running_from_executable() -> bool:
    """Determine if the Python script is running as an executable."""
    return getattr(sys, 'frozen', False)

class RedirectOutput:
    """Context manager to redirect stdout and stderr to /dev/null."""

    def __enter__(self) -> 'RedirectOutput':
        if is_running_from_executable():
            self.old_stdout = sys.stdout
            self.old_stderr = sys.stderr
            null_stream = open(os.devnull, 'a')
            sys.stdout = sys.stderr = null_stream
        return self

    def __exit__(self, exc_type: type, exc_value: Exception, traceback: type) -> None:
        if is_running_from_executable():
            sys.stdout.close()
            sys.stderr.close()
            sys.stdout = self.old_stdout
            sys.stderr = self.old_stderr

def redirect_stdout_stderr() -> RedirectOutput:
    """Get the RedirectOutput instance for use as a context manager."""
    return RedirectOutput()
  • Open /dev/null once and assign it to both stdout and stderr to avoid opening it twice.
  • __enter__ properly returns an instance of the class itself ('RedirectOutput').
  • __exit__ ensures that the original streams are restored even if an exception occurs within the block.

openadapt/utils.py Show resolved Hide resolved
@KIRA009 KIRA009 force-pushed the feature/refactor-config branch from f269ecf to 1ae5c75 Compare April 24, 2024 10:35
@abrichr
Copy link
Member

abrichr commented Apr 29, 2024

@KIRA009 there appears to be a regression affecting the pipes at the end of a recording:

bug.mov

The progress bar appears to complete before all events are saved. This behavior does not happen on main.

@KIRA009 KIRA009 force-pushed the feature/refactor-config branch from 1b4d52b to d669d08 Compare April 30, 2024 04:57
@KIRA009 KIRA009 force-pushed the feature/refactor-config branch from d669d08 to 7c8345f Compare April 30, 2024 05:05
Copy link
Member

@abrichr abrichr left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@abrichr abrichr merged commit ee506f0 into OpenAdaptAI:main May 1, 2024
1 check passed
@abrichr abrichr mentioned this pull request May 15, 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