Enforcing limits on the internal memory needed by monitors #1273
Conversation
tidy3d/components/simulation.py
Outdated
|
|
||
| return data_size | ||
|
|
||
| def _monitor_data_size(self, monitor): |
There was a problem hiding this comment.
Can you add type annotations?
tidy3d/components/simulation.py
Outdated
| # internal storage also does not exceed the limit. | ||
| check_monitor_types = [AbstractModeMonitor, SurfaceIntegrationMonitor, DiffractionMonitor] | ||
| for monitor in self.monitors: | ||
| if not isinstance(monitor, tuple(check_monitor_types)): |
There was a problem hiding this comment.
Not necessary to convert to tuple here i think
tidy3d/components/simulation.py
Outdated
| if not isinstance(monitor, tuple(check_monitor_types)): | ||
| continue | ||
|
|
||
| mnt_kwargs = { |
There was a problem hiding this comment.
I think a cleaner way to do this could be to add a property for monitors that returns the equivalent of the backend field monitor that would be used for that type of monitor (before processing) and then compute the storage size of that monitor here in the validator. What do you think? This might also be useful on the backend potentially or for debugging purposes
So I was thinking about doing it like this initially but it doesn't make that much sense actually; it doesn't work exactly like that on the backend. For example, surface integration monitors do not actually add a FieldMonitor that is then post-processed; on the other hand, ModeMonitor-s do. Also even disregarding that it wasn't easy for me to come up with a way to add this in a nicely unified way that would not need this property to be overwritten in multiple classes. I've modified things to just introduce a server storage method. What do you think about this? I also added a limit on the max data that a mode solver call can produce, because more than 20GB is likely to error currently. Finally, I also realized that the previous implementation was not right for TimeMonitor-s (essentially FluxTimeMonitor is the only one affected) because it was making a FieldTimeMonitor with the same amount of time steps; instead, the fields at a single time step are stored internally at any given time (this also goes to the above point that there's no direct correspondence to a FieldMonitor or FieldTimeMonitor). |
momchil-flex
force-pushed
the
momchil/monitor_limits
branch
from
d1af540
to
990630b
Compare
|
Can the current limit ensure OOM never occurs? |
Unfortunately no. There's too many ways in which it can happen and we need more work on server-side resource allocation too. But this will prevent some cases which would otherwise almost certainly have gone oom. |
tidy3d/components/simulation.py
Outdated
| for monitor in self.monitors: | ||
| num_cells = self._monitor_num_cells(monitor) | ||
| # intermediate storage needed | ||
| internal_data_gb = monitor._storage_size_solver(num_cells=num_cells, tmesh=self.tmesh) |
There was a problem hiding this comment.
would be a bit easier to understand if we named the variables like
internal_data_bytes = monitor._storage_size_solver(num_cells=num_cells, tmesh=self.tmesh)
internal_data_gb = internal_data_bytes / 2**30also in the other instance where the 2 ** 30 is divided out.
momchil-flex
force-pushed
the
momchil/monitor_limits
branch
from
cd7c3d2
to
33a2451
Compare
Currently, monitors that produce a small amount of data but may need a large amount of memory internally are not really validated to prevent OOM on our servers. I was thinking on imposing some limits like max number of freqs, or product of num_freqs x num_points x num_modes for mode monitors, etc. but I realized what probably makes the most sense is restricting the data size of the associated field monitor, which is always needed internally.