Skip to content

Launcher / Log Update #234

Merged
zangjiucheng merged 16 commits intomasterfrom
p22gu/182-launcher-args
Dec 26, 2024
Merged

Launcher / Log Update #234
zangjiucheng merged 16 commits intomasterfrom
p22gu/182-launcher-args

Conversation

@patrick-gu
Copy link
Member

@patrick-gu patrick-gu commented Jun 14, 2024

Description

I changed X and Y to accomplish Z.

This PR closes #182 .

Developer Testing

Here's what I did to test my changes:

  • Ran A
  • Ran B
  • Ran C

Reviewer Testing

Here's what you should do to quickly validate my changes:

  • Run D and check output

This change is Reviewable

patrick-gu and others added 6 commits June 10, 2024 09:13
* Added Save on Exit Prompt, details on Issue #138

* Small logic changes

* Updated saving mechanism to compare widgets only

* Rought implementation of new popup

* Fixed minor saving bugs and logic

* Small update to checkbox

* Minor bug fix

* Few logic changes and bug fixes

* Added additional clarity to comments

* Few renaming and logic changes

* close popup when user clicks X

* Merge saving updates; unify file location

* Small change to one comment

---------

Co-authored-by: Patrick Gu <patrick@patrickgu.ca>
@zangjiucheng
Copy link
Member

I will continue to work for this PR, which I believe is something we really need right now.

@zangjiucheng zangjiucheng changed the title Launcher args Launcher / Log Update Oct 11, 2024
@zangjiucheng zangjiucheng marked this pull request as ready for review October 11, 2024 04:13
@zangjiucheng
Copy link
Member

zangjiucheng commented Oct 11, 2024

Description

  • This is an enhance implement of Add Argument Support to Launcher #182, where the launcher has been used completely outdate of our code.
  • This feature implements the functional use cases of the new version of the launcher.
  • Finish implement of launcher arg support based on Patrick previous modify.
  • Support stdout to the screen but not redirect to the log files
  • Support --text mode argument input
  • Modify log file name with temp_stamp for permeant storage log for analysis.
  • Tiny fix for log tool with args index for general case

This PR closes #182 .

Known Issue

  • Stdout feature control not support --text version, (For purpose to use cli we want to see output, then just Enable stdout by default)

Developer Testing

Here's what I did to test my changes:

  • Run any combination with launcher to test if program can functional work.

Reviewer Testing

Here's what you should do to quickly validate my changes:

  • Same as Developer Testing

Copy link
Member Author

@patrick-gu patrick-gu left a comment

Choose a reason for hiding this comment

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

Left some comments about the code. Also it doesn't seem to work for me, I get the error:

Running in GUI mode
Launching... Run commands: [['venv/Scripts/python', '-m', 'omnibus', False], ['venv/Scripts/python', 'sources/fakeni/main.py', False], ['venv/Scripts/python', 'sources/parsley/main.py', '--fake', False], ['venv/Scripts/python', 'sinks/dashboard/main.py', True], ['venv/Scripts/python', 'sinks/printer/main.py', False]]
Traceback (most recent call last):
  File "...\omnibus\launcher.py", line 409, in <module>
    main()
  File "...\omnibus\launcher.py", line 403, in main
    gui_launcher.subprocess()
  File "...\omnibus\launcher.py", line 159, in subprocess
    process = subprocess.Popen(launch_command, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Program Files\Python312\Lib\subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "C:\Program Files\Python312\Lib\subprocess.py", line 1538, in _execute_child
    hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [WinError 2] The system cannot find the file specified

max_logs = args.max_logs
replay_speed = args.replay_speed
log_file = args.log_file if args.log_file != None else get_replay_log(max_logs)
log_file = os.path.expanduser(args.log_file if args.log_file != None else get_replay_log(max_logs))
Copy link
Member Author

Choose a reason for hiding this comment

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

is there a reason for this?

Copy link
Member

Choose a reason for hiding this comment

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

I did this because for some reason, if I use ~ relative to the home directory on a unix-like system, it pops up an error that the system cannot find the specified file. I think the reason for the error popping up before you mentioned is probably because I switched this... I think I need to add an OS check for this.

launcher.py Outdated
checkbox.stateChanged.connect(self.update_selected)
for i, (checkbox, args) in enumerate(self.src_widgets):
# https://docs.python.org/3/faq/programming.html#why-do-lambdas-defined-in-a-loop-with-different-values-all-return-the-same-result
def stateChanged(state, i=i):
Copy link
Member Author

Choose a reason for hiding this comment

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

This is really small nitpick. Although the Python documentation officially suggests you can do this, I would be inclined to avoid this. The reason is because we don't know if .connect will always only give us one argument, and I assume that nobody really wants to take the time to check (and just looking at the [outdated] docs quickly is unclear https://doc.qt.io/qtforpython-5/PySide2/QtCore/Signal.html#PySide2.QtCore.Signal.connect). I think it's better to make sure it's clear that this is a function with arity 1 (i.e. takes exactly one argument).

Copy link
Member

@zangjiucheng zangjiucheng Oct 28, 2024

Choose a reason for hiding this comment

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

Hi :)) I didn't think about a good way to pass only one argument because I have no idea how to edit state binding to a list. But I did use another solution with partial function to ensure it take two argument.

@zangjiucheng
Copy link
Member

I did test this script on the Windows and Linux also, unfortunately I cannot reproduce the issue [WinError 2] The system cannot find the file specified. Can you try again please? Maybe it been fixed in some comments I don't really noticed. Thanks for your review :))

@zangjiucheng zangjiucheng requested a review from a team as a code owner November 3, 2024 02:23
@zangjiucheng zangjiucheng merged commit 8166561 into master Dec 26, 2024
1 check passed
@zangjiucheng zangjiucheng deleted the p22gu/182-launcher-args branch December 26, 2024 18:46
@patrick-gu
Copy link
Member Author

Sorry I never responded to this! I've figured out that the issue was that I created my venv in a folder called .venv instead of venv. It shouldn't be an issue since the recommended setup instructions are compatible with this. Also, I haven't tested it thoroughly, but it does seem to work, so thank you :)

@zangjiucheng
Copy link
Member

@patrick-gu No worries :). I just get some free time today to review some code before. I did test more with more combination of sources and sinks. It should be working well! If any others come up, we can still fix it later lol. Merry Christmas 🎄

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.

Add Argument Support to Launcher

3 participants