refactor: extract VariableDetector base class for variable detectors#235
Conversation
Factor the ~90% duplicated lifecycle (train/detect/configure/post_train/ set_configuration) shared by the five variable-value detectors into a new abstract VariableDetector base in common/variable_detector.py. Each concrete detector (new_value, charset, value_range, bigram, new_value_combo) is reduced to a config subclass plus a few override hooks; all remain in the detectors package so name-based tracker reconstruction keeps working. Also fixes latent bugs surfaced by the refactor: - ValueRangeDetector now registers persistency and preserves the persist config across set_configuration (both were previously dropped). - SingleStabilityTracker no longer starts a throwaway PersistencySaver when reconstructing a detector to recover its add_value closure (it would leak a saver thread per variable and clobber the real state file under persist). Behavior-preserving refactor guarded by the existing per-detector suites. Full suite: 545 passed. prek gate (mypy --strict, flake8, vulture, docformatter) clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ipmach
left a comment
ipmach
left a comment
There was a problem hiding this comment.
I added some comments, otherwise looks ok
There was a problem hiding this comment.
I notice that many tests have the imports inside the functions.
Is there in any specific reason why? Otherwise they must be place in the top of the document.
There was a problem hiding this comment.
I recently became a fan of lazy imports but you're in this case. Has been corrected
There was a problem hiding this comment.
great, can you do the same removing them from "TestValueRangeDetectorPersistFixes" and I am ok with merging :)
There was a problem hiding this comment.
Not part of this PR, but why there is a function inside the init? looks a little weird
There was a problem hiding this comment.
You're right. I already addressed it in this PR. I moved this functionality to the persistency and by using "partial" of functools
There was a problem hiding this comment.
Same here, why there is a function inside the init?
Also, do we ned the sys.exit(1)? This will stop the entire run of the program.
…iting Convert the add_value closure declared inside __init__ into a normal method on each variable detector (charset, value_range, bigram). Trackers now bind it via functools.partial instead of the __get__ descriptor trick; persistence format (add_value_fn name string + detector_config) unchanged. ValueRangeDetector._prepare_variables now raises ValueError on non-numeric input instead of calling sys.exit(1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@ipmach I your addressed your requested changes |
Move imports that were deferred into test methods for no reason up to the file header (PersistConfig, PersistencySaver, CoreParser, ChunkedEventDataFrame, unittest.mock.patch), and drop a redundant local SingleStabilityTracker import already present at module scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Task
Description
Extracts an abstract
VariableDetectorbase class (common/variable_detector.py,inheriting
CoreDetector) that owns the shared lifecycle (train/detect/configure/post_train/set_configuration) previously duplicated ~90% across thefive variable-value detectors. Each concrete detector —
NewValue,Charset,ValueRange,BigramFrequency,NewValueCombo— is reduced to a config subclassplus a few override hooks (
_check_variable,_prepare_variables,_event_data_kwargs,_check_event,_description). All detectors stay in thedetectorspackage soname-based tracker reconstruction keeps working. Net: +439 / −712 lines.
new_event/random/ruleare untouched.Also fixes latent bugs surfaced by the refactor:
ValueRangeDetectornow registers persistency and preserves thepersistconfigacross
set_configuration(both were previously dropped).SingleStabilityTrackerno longer starts a throwawayPersistencySaverwhenreconstructing a detector to recover its
add_valueclosure (it would leak a saverthread per variable and clobber the real state file under
persist).Behavior-preserving; the only intended behavior changes are the two
value_rangefixes.How Has This Been Tested?
regression test that reconstruction under
persiststarts no throwaway saver.prekgate (mypy--strict, flake8, vulture, docformatter,bandit) clean.
Checklist