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

Fix : Local development failing due to changed function definition #776

Conversation

shashank40
Copy link
Contributor

@shashank40 shashank40 commented Jun 18, 2024

What kind of change does this PR introduce?
fixes #773

Summary

The current local development setup is buggy due to recent code changes. The helper function definitions were changed but the usage was not updates.
Ex: functions for recording start, replay etc

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?

Other information

@@ -24,7 +25,7 @@ def main() -> None:
subprocess.run(["git", "pull", "-q"])
logger.info("Updated the OpenAdapt App")

run_app() # start gui
start() # start gui
Copy link
Member

@abrichr abrichr Jun 18, 2024

Choose a reason for hiding this comment

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

@shashank40 thank you for putting this together!

@KIRA009 do you know whether we are using this file openadapt/start.py anywhere? Git blame shows last update was about a year go

@shashank40 I think this file is deprecated. Can you please clarify what caused you to attempt to run it? If it's documented somewhere we should clarify that it is deprecated.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are not. This script looks like it updates the state of your local repo, but I don't think this is needed any more now that we have apps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abrichr @KIRA009 as my app was not working after download, i used this script to spin up the app.

python -m openadapter.start runs a client for us and that is what i used this for

Copy link
Member

Choose a reason for hiding this comment

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

@shashank40 please feel free to move this file to openadapt/deprecated, and remove any mention of it in any documentation that you encountered 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@shashank40 Sorry for being confused, but when you say the app wasn't working in the first instance, did you mean the compiled version (the link you find on https://openadapt.ai/) or did you set it up manually from the git repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KIRA009
I meant the compiled app as i commented here.
#771 (comment)

openadapt/plotting.py Outdated Show resolved Hide resolved
@@ -35,20 +35,20 @@ class VanillaReplayStrategy(strategies.base.BaseReplayStrategy):
def __init__(
self,
recording: models.Recording,
replay_instructions: str = "",
instructions: str = "",
Copy link
Member

Choose a reason for hiding this comment

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

@shashank40 if you remove the rest of this PR and only include the changes to this file I will be happy to merge it 👍

@shashank40 shashank40 force-pushed the fix/local_development_record_replay_failing branch from 77b1e26 to 8418f03 Compare June 18, 2024 15:12
@@ -195,7 +195,7 @@ def generate_action_event(
current_window=current_window_dict,
recorded_actions=recorded_action_dicts,
replayed_actions=replayed_action_dicts,
replay_instructions=replay_instructions,
instructions=instructions,
Copy link
Member

Choose a reason for hiding this comment

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

I believe this needs to stay as "replay_instructions", as per the .j2 file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But replay fails.
If we use the replay command with --instructions, it fails with the error, undefined argument instructions.

We can do 2 things
either the command we use has--replay_instructions as argument OR vanilla.py changes replay_instructions -> instructions.

I would change it to whatever you might prefer

Copy link
Member

@abrichr abrichr Jun 18, 2024

Choose a reason for hiding this comment

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

To clarify:

this line should read replay_instructions=instructions

This should not produce any error. If it does, please post the full log output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yess, it would, missed the line number. You are correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abrichr updated the PR 🙏🏻

@shashank40 shashank40 force-pushed the fix/local_development_record_replay_failing branch from 8cfe023 to 4f491fd Compare June 18, 2024 15:32
@shashank40 shashank40 force-pushed the fix/local_development_record_replay_failing branch from a3fc674 to 014891a Compare June 18, 2024 17:40
@shashank40 shashank40 force-pushed the fix/local_development_record_replay_failing branch from 014891a to 98e74cd Compare June 18, 2024 18:38
@shashank40 shashank40 closed this Jun 18, 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.

[Bug]: Local development record, replay and start fail
3 participants