For non-interactive usage, such as headless Docker, use_stty_size setting for Analyze

[guydavis]

For non-interactive usages where a TTY is not available, such as inside a Docker container when invoked by Python process, this allows the existing setting of use_stty_size to be considered when performing Analyze and other options.

In these cases where use_stty_size: False, this will avoid the need to run Docker container with a TTY attached. In particular, on TrueNAS via their helm chart deployment, they aren't able to enable TTY, thus preventing them from invoking Plotman via the Machinaris WebUI.

As the use_stty_size: True default setting remains in-place as before, all regular console/interactive usage of Plotman is unchanged by this PR.

[altendky]

Sorry I left this hanging. Any chance you could take a pass at creating a single function to handle this? There were still a couple dangling references that I guess we missed the last time we touched this. If needed, a new util file could be created to hold that function.

[guydavis]

Sorry I left this hanging. Any chance you could take a pass at creating a single function to handle this? There were still a couple dangling references that I guess we missed the last time we touched this. If needed, a new util file could be created to hold that function.

Appreciate the feedback. As this diff removes the other invocation of stty, leaving only the one invocation within the get_term_width method, do you mean I should move the get_term_width method out of plotman.py?

Also, could you help me understand the failing typing issue for the parameter introduced into that method? I tried provided a typing for cfg but it didn't seem to pass Github Actions checks.

Thanks!

[altendky]

At least this... ​but I think there may have been one or two more spots.

https://github.com/guydavis/plotman/blob/77f85e3b38abc45d8cb6af3bad650b458c3aef35/src/plotman/interactive.py#L156-L173

       ​# Get terminal size.  Recommended method is stdscr.getmaxyx(), but this
       ​# does not seem to work on some systems.  It may be a bug in Python
       ​# curses, maybe having to do with registering sigwinch handlers in
       ​# multithreaded environments.  See e.g.
       ​#     https://stackoverflow.com/questions/33906183#33906270
       ​# Alternative option is to call out to `stty size`.  For now, we
       ​# support both strategies, selected by a config option.
       ​# TODO: also try shutil.get_terminal_size()
       ​n_rows: int
       ​n_cols: int
       ​if cfg.user_interface.use_stty_size:
           ​completed_process = subprocess.run(
               ​['stty', 'size'], check=True, encoding='utf-8', stdout=subprocess.PIPE
           ​)
           ​elements = completed_process.stdout.split()
           ​(n_rows, n_cols) = [int(v) for v in elements]
       ​else:
           ​(n_rows, n_cols) = map(int, stdscr.getmaxyx())

We had not gotten all the terminal size checking centralized before. I think this subprocess.run() code could be put in a get_term_size() function to get both the height and the width and located in one place (maybe a plotman.util if you don't see a better option).

Instead of a hard coded 120 default, perhaps it would be good to take a CLI option to bypass the detection and set the width. That would both allow you to force it not be detected and also control the width.

These things don't all have to happen get this merged, but I figured I'd see if you were up for a bit more work. :]

For the type hints, you did fix them but you didn't correct the formatting check. If you have tox installed you should be able to tox -e format to get it to apply the needed formatting. Or use black directly if you prefer. Or, for that matter, manually apply the changes below.

https://github.com/ericaltendorf/plotman/runs/3579679794#step:10:12

--- src/plotman/analyzer.py	2021-09-12 13:34:41.444647 +0000
+++ src/plotman/analyzer.py	2021-09-12 13:35:05.902098 +0000
@@ -8,11 +8,15 @@
 
 from plotman import plot_util
 
 
 def analyze(
-    logfilenames: typing.List[str], clipterminals: bool, bytmp: bool, bybitfield: bool, columns: int
+    logfilenames: typing.List[str],
+    clipterminals: bool,
+    bytmp: bool,
+    bybitfield: bool,
+    columns: int,
 ) -> None:
     data: typing.Dict[str, typing.Dict[str, typing.List[float]]] = {}
     for logfilename in logfilenames:
         with open(logfilename, "r") as f:
             # Record of slicing and data associated with the slice
would reformat src/plotman/analyzer.py
--- src/plotman/plotman.py	2021-09-12 13:34:41.444647 +0000
+++ src/plotman/plotman.py	2021-09-12 13:35:06.829178 +0000
@@ -295,11 +295,15 @@
         # Analysis of completed jobs
         #
         elif args.cmd == "analyze":
 
             analyzer.analyze(
-                args.logfile, args.clipterminals, args.bytmp, args.bybitfield, get_term_width(cfg)
+                args.logfile,
+                args.clipterminals,
+                args.bytmp,
+                args.bybitfield,
+                get_term_width(cfg),
             )
 
         #
         # Exports log metadata to CSV
         #

[altendky]

Sorry this missed the release, we can do another small one soon.

[guydavis]

Great, formatting checks all pass now.

[altendky]

Thanks as always.