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

Rich console width #1070

Merged
merged 1 commit into from
Aug 17, 2023
Merged

Rich console width #1070

merged 1 commit into from
Aug 17, 2023

Conversation

dbochkov-flexcompute
Copy link
Contributor

Here's how we can adjust width of rich output, so that it should eliminate horizontal scroll bars in web documentation. Perhaps an alternative that we can consider is to define this width in the config, so that it could be changed as needed? What do you think? @tylerflex @momchil-flex

@tylerflex
Copy link
Collaborator

tylerflex commented Aug 16, 2023

Looks good. This will be great.

What worries me slightly is remembering to pass width=CONSOLE_WIDTH in the future for all progress bars or consoles.

Would it make sense to define a console instance in log.py and import that?

console = Console(width=CONSOLE_WIDTH)

or alternatively, if we need to make a new console each time, we could try:

# log.py
def make_console(*args, **kwargs):
    if "width" not in kwargs:
        kwargs["width"] = CONSOLE_WIDTH
    return Console(*args, **kwargs)

and call that function in the webapi?

@dbochkov-flexcompute
Copy link
Contributor Author

yeah, that sounds better. In fact, internally rich has a similar logic https://github.com/Textualize/rich/blob/master/rich/__init__.py#L23-L36. Probably we can just use it whenever we need a console, just call rich.reconfigure(width=CONSOLE_WIDTH) somewhere during tidy3d initialization

@dbochkov-flexcompute
Copy link
Contributor Author

So, I realized we already create a console instance inside our logger, and decided just to retrieve it anytime we need a rich console. Do you see any issues with that?

@tylerflex
Copy link
Collaborator

tylerflex commented Aug 17, 2023

Not specifically. As long as it works properly when running different notebooks for example it should be fine. For example if notebook A and B are running simultaneously, their outputs shouldn't get mixed up

@dbochkov-flexcompute
Copy link
Contributor Author

dbochkov-flexcompute commented Aug 17, 2023

Yeah, doesn't seem to break anything. Just noticed that rich.track() should also be given a console, added that.

This is what I get after compilation of html. Maybe we should also consider setting code width to 80 when black'ing notebooks
console_width1
console_width2
console_width3

@dbochkov-flexcompute dbochkov-flexcompute marked this pull request as ready for review August 17, 2023 15:26
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks! just a minor comment to explain the CONSOLE_WIDTH variable for future reference and also could you add a CHANGELOG item? Thanks!

@@ -32,6 +32,8 @@
"CRITICAL": "red bold",
}

CONSOLE_WIDTH = 80
Copy link
Collaborator

@tylerflex tylerflex Aug 17, 2023

Choose a reason for hiding this comment

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

can you just add a comment above this saying that

# Width of the console used for rich logging (in characters).

@momchil-flex
Copy link
Collaborator

Can you just double check that the logging level and verbose in web functions are still independent from one another. I think they should be but just to make sure.

@dbochkov-flexcompute dbochkov-flexcompute merged commit c08c157 into pre/2.4 Aug 17, 2023
16 checks passed
@tylerflex
Copy link
Collaborator

I'm excited to see the newly rendered docs after this fix :D

@dbochkov-flexcompute dbochkov-flexcompute deleted the daniil/console-width branch September 5, 2023 23:59
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.

3 participants