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

Add option to use dispersion fitter without rich.progress #2258

Draft
wants to merge 1 commit into
base: pre/2.8
Choose a base branch
from

Conversation

caseyflex
Copy link
Contributor

The rich package is not supported by GUI, so they requested the option to use the fitter without this package.

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Thanks @caseyflex yeah I think we need to generally streamline sources/sinks for logging etc., this is kind of related to that.

A lazy import for rich is the right choice (as we do elsewhere), what I'm a bit concerned about is the duplication of logic in the fit() function, it's more surface area for bugs to creep in. I think a more robust approach might be to keep the current logic as-is, but introduce a dummy progress bar that exposes all the same methods but does nothing if the rich import fails. This could in principle also be reused in other functions. We could either add it here, or perhaps better in tidy3d/log.py (although we import rich there too so this ends up being a bit of a rabbit hole...).

Something like:

class NoOpProgress:
  def __enter__(self): return self
  def __exit__(self, *args, **kwargs): pass
  # any other methods that are called

try:
  from rich.progress import Progress
except ImportError:
  Progress = NoOpProgress

Depending on your bandwidth I think it'd be ok to define the dummy within the context of the function for now (this is mostly an MC thing right?), but we should aim for a more complete solution down the line. What do you think?

@caseyflex caseyflex marked this pull request as draft February 20, 2025 10:17
@caseyflex
Copy link
Contributor Author

Thanks @yaugenst-flex

Converting this to draft as we need to clarify the requirements here. I now see that even base.py imports rich, so it might be that MC wants all dependencies on Tidy3d classes removed from the fitter. I have some thoughts about improving the fitter speed at the same time so it might be worth it, although it is a large effort. But first we need to clarify exactly what they want. Scipy is a pretty essential dependency at the moment too, which may be too heavy.

Regarding the general streamlining -- let's say we don't worry about imports for now. It would be nice to introduce classes in log.py to wrap rich Progress. In particular it would be nice to control whether a progress bar appears or not in the same way as our log_level, although possibly with a different setting because it's a bit independent from the rest of the logging.

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.

2 participants