feature/bgunnar5/status-pager

[bgunnar5]

Losing my mind with celery atm so I took a break and added the status pager functionality for you. This is my first contribution to Maestro so let me know if there's anything else I need to do that I haven't yet.

[FrankD412]

@bgunnar5 -- Thanks for the contribution! I'll review it this week and get back to you. For now, the changes at a glance seem reasonable, I'd just like to test how this behaves locally. Also, it looks like the doc building step in the testing failed so that needs to pass before we can merge this in.

[bgunnar5]

Sounds good.

Regarding the docs test failing, it seems to have an import error with the virtualenv package (which shouldn't be a result of anything I modified):

ImportError

  cannot import name 'Python2Supports' from 'virtualenv.create.describe' (/home/docs/checkouts/readthedocs.org/user_builds/maestrowf/envs/420/lib/python3.10/site-packages/virtualenv/create/describe.py)

  at ~/checkouts/readthedocs.org/user_builds/maestrowf/envs/420/lib/python3.10/site-packages/virtualenv/create/via_global_ref/builtin/python2/python2.py:6 in <module>
        2│ import json
        3│ import os
        4│ from pathlib import Path
        5│ 
    →   6│ from virtualenv.create.describe import Python2Supports
        7│ from virtualenv.create.via_global_ref.builtin.ref import PathRefToDest
        8│ from virtualenv.info import IS_ZIPAPP
        9│ from virtualenv.util.zipapp import read as read_from_zipapp
       10│

@jwhite242 I think this is the same error that Lina is getting in the weave_ci pipeline when she's trying to build the docs. Any idea how to resolve this?

[jwhite242]

Maybe.. so the error she had was a different import error, and this one it's not clear what's causing it as the import error message gets injected randomly in poetry's stdout. I was able to get around it by pinning poetry to 1.4.2 for now, which the readthedocs build doesn't do (1.5.x fails for me with the same error). So try changing this line to pin poetry to that version:

maestrowf/.readthedocs.yaml

Line 9 in f5fcad5

- pip install poetry>=1.2

[jwhite242]

One question on the pager/theme issues you'd found: is there an exception that gets thrown, or is it just a terminal emulator error that we can't do anything about in here? Meaning, there's no sort of try/except block we can do to auto-rollback the use styles flag in the pager here?

[bgunnar5]

I haven't seen any errors thrown when using pager and theme together. If the pager doesn't accept customization it'll just display something like this in the terminal window:

I don't believe there's a way to try/except catch this. I did add a message in the maestro status --help page for the --disable-theme description talking about how setting MANPAGER or PAGER to be "less -r" may be the fix for this. Maybe I could add a little printed message to be displayed on each status run that says this instead so users don't have to go searching for it?

[doutriaux1]

@bgunnar5 maybe here? https://stackoverflow.com/questions/7445658/how-to-detect-if-the-console-does-support-ansi-escape-codes-in-python

[bgunnar5]

this specific issue is weird @doutriaux1 because if you don't use the pager functionality then the ANSI colors work just fine, so I'm not quite sure if this relates to stdout/stderr or if this is something different entirely

[jwhite242]

I haven't seen any errors thrown when using pager and theme together. If the pager doesn't accept customization it'll just display something like this in the terminal window:

I don't believe there's a way to try/except catch this. I did add a message in the maestro status --help page for the --disable-theme description talking about how setting MANPAGER or PAGER to be "less -r" may be the fix for this. Maybe I could add a little printed message to be displayed on each status run that says this instead so users don't have to go searching for it?

Ok, thanks for checking on the error source. Just want to make sure we catch things when we can to give better error messaging to the users

[jwhite242]

So I think the only thing really missing now is adding these options to the docs. The cli args for sure need to get put in here: https://github.com/LLNL/maestrowf/blob/f5fcad59a697a389359b00c8faf1576050924404/docs/Maestro/cli.md?plain=1

But maybe also one of the admonitions (https://squidfunk.github.io/mkdocs-material/reference/admonitions/) would be good in the montoring section of the user guide (https://github.com/LLNL/maestrowf/blob/f5fcad59a697a389359b00c8faf1576050924404/docs/Maestro/monitoring.md?plain=1) to mention what to do if things look garbled. Not sure which one's most appropriate here, Tip, Note, Info,... or maybe even Warning?

[bgunnar5]

Good call, can't believe I forgot docs! I added a note admonition in the monitoring section and two sections for the theme and pager additions that you should easily be able to expand on in the future if you'd like.

[FrankD412]

@jwhite242 @bgunnar5 -- sorry for my silence here, been focused elsewhere. I have time off this weekend starting today and plan to catch up here. Looks like @jwhite242 has it in hand though 😄

[bgunnar5]

No worries! Jeremy and I were discussing these changes a bit before I even made this branch so he's pretty up-to-date on everything I've been doing here 😄

[jwhite242]

So, just two minor comments/changes and then think we can get this in:

can you add a short mention that this is usually tied to the 'less' command which also enables some extra help/navigation/etc options; don't need to enumerate them I think, as telling users what command is running and that they are there gives them the info to find all this out from the authoritative source.

also, after merging in develop, can you tick the dev version too? should be on 1.1.10dev3 with this one (apologies for the manual bits here; working on some better ci based management for this to make it easier and more robust).

and thanks again for contributing this! this is going to be great for large studies!