-
Notifications
You must be signed in to change notification settings - Fork 40
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 cache functionality to Logger #895
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, this will give us some nice flexibility in how we display and package our validations. I just had a few higher level questions so I can understand a bit better
- Does the caching happen based on the message string itself? because in our simulation.py code, for example, our warning message will contain information like the index into
structures
, which means that the message could be different each iteration. I think it would therefore not work as expected with this change? For example, we might want a way to display only the first warning within a context and then suppress the rest (but not necessarily cache the result). Right now, we've just been breaking out of the loop manually, which works. - I'm having trouble understanding the
use_static_cache
option. Is the point that if this isFalse
, the caching happens at the global level (ie any logging gets cached and retained throughout the session) but ifuse_static_cache=True
, the caching only happens within the context of the context manager? Maybe the "static" nomenclature is what's confusing me.
after looking at the example again, I think I'm still a bit confused, could you explain a bit what is going on here? For example, in the first few example is it just displaying the first log but then suppressing the rest? In the use_static_cache, it seems like similar behavior but just not printing an INFO message about suppression. Where does caching come in? For example, does the first |
The idea is the following: caching (either version) only takes place within managed context, both for storing entries and querying the contents of the cache. So the first info message in the example should have no effect anywhere else, nor any messages outside cached contexts (the last info message, for example). We could repeat this info messages everywhere and the outputs cached should remain the same. That's because we don't want to have a global state having effects throughout the code base. The default (non-static) cache works only within a single context. It only allows a single log entry to be emitted, whatever level or message. All other entries after the first are accumulated (by level) so that a summary can be emitted after the context finishes (that was something Xin said might be valuable). The idea here is to substitute those early exits in warning loops you mentioned. In this case, the user will see the 1st warning as before and the total number of warnings emitted. So, for example, is a simulation validation generates 10 warnings about structures, the user sees the first and a message saying another 9 warnings were suppressed. That way they know how many issues have been found. The way I implemented, each validator has their own context, so the user will receive 1 message from each validator warning. The static cache issues only messages that have not been issued before and is global to all cached contexts. The use here is to warn the user, for example, that we have now a remote mode solver on the first use of the local one, but never again. That will also be useful for any deprecation warnings we want to issue, so the user is not bombarded by recurring warnings. Caching by message contents happens only for the static case, where messages should probably be constant. If not, the best way to ensure caching with dynamic strings, is to use the log system to do the composition: |
Ok I see, this sounds great. So the |
Agreed! I also think that using 2 separate functions makes much more sense! I'll make the changes soon. I implemented the new log contexts to |
in the adjoint.components.simulation there are some validators that could probably also use it. they're similar to the ones in the regular Simulation. |
8e9f50d
to
66a72d5
Compare
This sounds great, thank for merging these two functionalities, it seems to be working as expected. For using capturing, maybe we can do something like that:
I can take a stab at it and commit/PR |
The way is works now is on self._log_trace = stack_item
def last_trace(self):
trace = self._log_trace
self._log_trace = None
return trace and the client service would just look at that |
by |
Ah, I see! Yes, looks great to me! The test is a good addition too! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from a high level, so is there a summary of the changes to the logger somewhere? I feel like it would be really useful to have some kind of basic developer's guide to the new logging features, this could even be written in the form of a test. Is the main change just that
with log as captured_log:
consolidates all the messages? and more generally that any logging within a Tidy3DBaseModel init gets captured?
What happened to the functionality for eg the mode solver where we wanted to log a warning only once per session? is that included in the caching or a separate
Yes, this context manager The global log suppression if done with the keyword argument Capturing for the UI happens regardless of the other functions, so the UI is unaffected by them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds great! I'll approve but maybe @dbochkov-flexcompute wants to do a final review? note, you'll probably need to rebase this against pre/2.3 as I merged develop into pre/2.3 to incorporate tox and other fixes
Everything looks good to me too, I just added a clean test for warning capturing functionality. I can go ahead and rebase to pre/2.3 |
The cache can be used to suppress multiple messages from a context manager within that context or globally, for the whole python process. Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com>
1561229
to
c15fadc
Compare
No issues during rebasing. Do we want to squash some/all commits in here? it kinda got out of hands lol. Up to you, @lucas-flexcompute |
The logger can be used for suppressing and capturing messages: - Message suppression from a global cache with `log_once` - Message consolidation within a context manager (to suppress multiple messages generated in loops) - Message capture in a tree-like structure for presentation in the Web UI Signed-off-by: Lucas Heitzmann Gabrielli <lucas@flexcompute.com> Co-authored-by: dbochkov-flexcompute <daniil@flexcompute.com>
The cache can be used to suppress multiple messages from a context manager within that context or globally, for the whole python process. Here's a preview of how it works:
Output: