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

Show WebAPI system warning message. #2075

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Show WebAPI system warning message. #2075

merged 1 commit into from
Nov 25, 2024

Conversation

QingengWei
Copy link

No description provided.

@QingengWei
Copy link
Author

QingengWei commented Nov 18, 2024

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is just copied directly from tidy3d/log.py? I think this is not a good idea since we will need to maintain two identical files. is there a better approach?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we can move tidy3d/log.py to tidy3d/core/log.py.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem with this is that the log is used internally in the tidy3d components. So if we move it to web then we would be forced to import web (and do authentication and other things) to use tidy3d. It wold be nice if they were separate. What about just using the rich console to do the logging here? like we do in web.monitor?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am actually confused, why is this a problem? tidy3d/log.py is top level for both the components and web modules, why can't both just import it?

Copy link
Author

Choose a reason for hiding this comment

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

According to the original agreement, web/core is the bottom layer and cannot depend on other components, otherwise it will cause circular dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the original agreement, web/core is the bottom layer and cannot depend on other components, otherwise it will cause circular dependencies.

I see, but is this really a problem for the log or rather specifically for things in components/ and plugins/. I could see logic behind things we put in the main tidy3d folder to be usable by all other modules, i.e. both components and web. @tylerflex ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The way it was designed, the web API and tidy3d could be agnostic of one another. And just communicate through this "stub" class. In principle I think we could import log into web/core, but for now I'd prefer using rich console to not break this architecture unless we really need to. In the future we can figure out a better long term solution to decouple these things. probably the web API needs to be its own package and it might just need to import tidy3d as a dependency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, now I see, I think then to me it sounds like this warning should ideally be propagated from web/core to the corresponding web/api function where it occurs, and just shown there using the usual log. But for the time being rich console sounds good.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I have changed to rich console.

Copy link
Author

@QingengWei QingengWei Nov 21, 2024

Choose a reason for hiding this comment

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

The way it was designed, the web API and tidy3d could be agnostic of one another. And just communicate through this "stub" class. In principle I think we could import log into web/core, but for now I'd prefer using rich console to not break this architecture unless we really need to. In the future we can figure out a better long term solution to decouple these things. probably the web API needs to be its own package and it might just need to import tidy3d as a dependency.

Great idea! We shouldn't embed web/* into tidy3d, as some users may not utilize the web functionality. Instead, we can separate web/* into its standalone package that depends on tidy3d. This way, users can choose the appropriate package according to their needs.

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.

Hi @magiWei Looks good overall but see how we do the logger in other parts of web/core

https://github.com/flexcompute/tidy3d/blob/develop/tidy3d/web/core/environment.py#L93-L106

let's use this get_logger() for consistency and so we can change the base config and have it reflect everywhere.

Also wonder if momchil's suggestion about just handing the warning in the web/api side makes some sense? but for now maybe this is sufficient
Thanks

@QingengWei
Copy link
Author

Hi @magiWei Looks good overall but see how we do the logger in other parts of web/core

https://github.com/flexcompute/tidy3d/blob/develop/tidy3d/web/core/environment.py#L93-L106

let's use this get_logger() for consistency and so we can change the base config and have it reflect everywhere.

Also wonder if momchil's suggestion about just handing the warning in the web/api side makes some sense? but for now maybe this is sufficient Thanks

Ok, it makes sense to me. Changed.

@momchil-flex momchil-flex merged commit 45f49e3 into pre/2.8 Nov 25, 2024
15 checks passed
@momchil-flex momchil-flex deleted the SCEM-8018 branch November 25, 2024 13:33
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