Add network_inspectors.py module in LANfactory#82
Conversation
Port network-inspector visualizations to LANfactory using ssms.basic_simulators.simulator, ssms.config.ModelConfigBuilder, ssms.support_utils.kde_class.LogKDE, and LoadTorchMLPInfer.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughA new Changesnetwork_inspectors module and wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lanfactory/__init__.py`:
- Around line 7-9: The eager import of network_inspectors at the top of the
__init__.py file forces scikit-learn to be loaded when the lanfactory package is
imported, even though sklearn is not in the project dependencies. Remove the
`from . import network_inspectors` statement on line 7 to prevent the transitive
import of sklearn for users who don't need the inspectors module. Additionally,
remove `"network_inspectors"` from the __all__ list on line 9, or if the module
needs to remain part of the public API, implement lazy loading for it instead of
eager importing.
In `@src/lanfactory/network_inspectors.py`:
- Line 121: The plt.subplots() calls at lines 121, 193, 206, 218, and 233 in
src/lanfactory/network_inspectors.py squeeze the axes when rows equals 1 or cols
equals 1, returning a 1D array instead of a 2D array. This causes the subsequent
2D indexing ax[row_tmp, col_tmp] to fail. Add the squeeze=False parameter to all
five plt.subplots() calls to ensure axes are always returned as a 2D array
regardless of the number of rows or columns, preventing indexing crashes.
- Around line 68-80: The kde_vs_lan_likelihoods function and other public
plotting functions accept parameter_df, model, and torch_mlp_predict with None
defaults but immediately dereference these values throughout the function body
(causing opaque runtime errors). Add early validation checks at the start of
each affected function to verify that these required parameters are not None,
and raise a clear, actionable ValueError or TypeError with a descriptive message
if they are missing, rather than allowing them to fail later with cryptic
attribute or type errors.
- Line 119: The sns.set() call on line 119 uses a hardcoded font_scale=2 value,
which ignores the font_scale parameter that is passed to the function (defined
on line 78). Replace the hardcoded font_scale=2 with the font_scale parameter
variable so that the caller's configuration is respected and takes effect in the
visualization settings.
- Around line 360-367: The code at line 360 calls .shape[0] on an element from
vary_dict, expecting it to be a numpy array, but the default value of vary_dict
uses Python lists instead, which lack the .shape attribute and cause an
AttributeError. Find where vary_dict is defined with its default value and
convert the default from using Python lists to using numpy arrays so that the
.shape[0] access at line 360 works correctly with default parameters.
- Around line 139-153: The input_batch array is initialized with a hardcoded
size of 4000 rows on line 151, but the plot_data array is dynamically sized to
len(config["choices"]) * 1000 rows. When len(config["choices"]) is not equal to
2, this causes a shape mismatch when attempting to assign plot_data to the
input_batch slicing operation on line 152. Replace the hardcoded 4000 value in
the input_batch initialization with len(config["choices"]) * 1000 to make the
row count dynamic and match the actual size of plot_data.
- Around line 14-21: The bare except clause at line 17 catches all exceptions
during the import of LoadTorchMLPInfer, not just ImportError. This masks the
actual error when import fails. Replace the bare except with except ImportError
to only catch import-related errors. Additionally, add a check in the
get_torch_mlp function (around line 59) before using LoadTorchMLPInfer to
validate that the import was successful, and raise an appropriate error if it
was not, rather than allowing a delayed NameError to occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 87480f93-721c-46ca-8e09-93545249dc66
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
pyproject.tomlsrc/lanfactory/__init__.pysrc/lanfactory/network_inspectors.py
cpaniaguam
left a comment
There was a problem hiding this comment.
This is a great start, thanks @mariama-design! Two things to consider:
-
I think it'd be a good idea to add a new version of the old tutorial -- either in ipynb notebook format or marimo. It'd be a nice way to interactively make sure everything works as it did before.
-
Later we should discuss adding test coverage to these features
There was a problem hiding this comment.
This file should not be included in the PR. Add it to the .gitignore file so Git doesn't track changes to it.
Port network-inspector visualizations to LANfactory using
ssms.basic_simulators.simulator,
ssms.config.ModelConfigBuilder, ssms.support_utils.kde_class.LogKDE, and
LoadTorchMLPInfer.
Summary by CodeRabbit
Release Notes
seabornto the project’s main dependencies to support improved plotting.