[bugfix] fix feature configs fg can not honor - #673
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| """Get fg json config impl.""" | ||
| combiner = self.config.combiner.lower() | ||
| # an empty combiner outputs the matched values without aggregation | ||
| if combiner and combiner not in FG_COMBINERS: |
There was a problem hiding this comment.
The combiner is validated unconditionally here, but on two paths below it is discarded before fg ever sees it: value_dim > 1 overrides it to "" (line 140), and every discrete/sparse lookup (needDiscrete — hash_bucket_size / vocab_* / num_buckets / boundaries / zch / dynamicemb) overrides it to "" (lines 190–191), matching the proto note "when need_discrete is true, combiner will be empty string".
A previously-working sparse lookup config carrying a stale gap_min/gap_max (values the docs advertised until now, and which fg never received because the field was zeroed) will now hard-fail on a field that is ignored. Consider validating only where the combiner is actually emitted, or moving the check after the overrides.
| MAX_HASH_BUCKET_SIZE = 2**63 - 1 | ||
| # combiners fg implements. gap_min/gap_max parse but are never accumulated, | ||
| # and fg falls back to sum for any other value without warning. | ||
| FG_COMBINERS = frozenset({"sum", "mean", "avg", "min", "max", "count"}) |
There was a problem hiding this comment.
FG_COMBINERS (and FG_OVERLAP_METHODS) are now the enforced source of truth, but the proto comments in feature.proto — a file this PR already modifies — contradict them: line 341 lists CombineFeature combiners as {sum | mean | min | max} (missing avg/count), line 396 lists LookupFeature's without avg, and line 632 lists only 4 of the 9 overlap methods. Deferring the docs/*.md updates to the doc-only PR makes sense, but these are comment-only edits with no wire impact, and a user hitting the new ValueError will read a proto comment that lists a different set than the error message. Worth syncing them here.
| optional string separator = 7 [default = "\x1d"]; | ||
| // fg kv separator, default is :. | ||
| optional float kv_delimiter = 8; | ||
| // fg kv separator, default is :. only one char is allowed |
There was a problem hiding this comment.
Nit: "default is :" reads like a proto-level default, but the field has no [default = ...] annotation (proto2 default is ""); : is fg's own fallback when the key is absent, which works because kv_dot_product.py guards emission with HasField. Since the line is being rewritten anyway, consider adding [default = ":"] to match the sibling separator = 7 [default = "\x1d"] (behavior is unchanged — emission is still HasField-gated), or rewording to "fg defaults to : when unset".
| "expression": self.config.expression, | ||
| "value_type": "float", | ||
| } | ||
| if self.config.separator != "\x1d": |
There was a problem hiding this comment.
Question: with separator now emitted for scalar features (value_dim == 1), if the input field actually contains the separator, fg will split it and can produce multiple values per row while the parser still assumes width 1 (data_parser.py records feature.value_dim in dense_length_per_key and pads to a fixed width). The old behavior for such configs was "separator silently ignored → unparseable value falls back to default", so this only affects previously-mishandled configs — but the new failure mode can be a shape mismatch downstream rather than a clear error. Does pyfg clamp the output width for value_dim == 1 here, or would rejecting separator != default with value_dim == 1 (consistent with this PR's validation theme) be safer?
| np.testing.assert_allclose(parsed_feat.values, np.array(expected_values)) | ||
| np.testing.assert_allclose(parsed_feat.lengths, np.array(expected_lengths)) | ||
|
|
||
| def test_overlap_feature_with_invalid_method(self): |
There was a problem hiding this comment.
The negative case is good, but 5 of the 9 whitelisted methods (index_of and the four proximity_*) have no coverage anywhere in this file. A typo in FG_OVERLAP_METHODS for one of them would hard-reject configs fg actually honors, with no test failing. Consider at least one parse case for a method with distinctive output (e.g. index_of, whose -1.0 miss value is easy to assert) to pin the whitelist to real fg behavior.
| np.allclose(parsed_feat.seq_lengths, np.array(expected_seq_lengths)) | ||
| ) | ||
|
|
||
| def test_lookup_feature_with_invalid_combiner(self): |
There was a problem hiding this comment.
Two suggestions for the combiner validation tests (applies to combine_feature_test.py:360 too):
- The whitelist itself is untested: existing parameterized tables only exercise
sum/mean(and""for lookup), so nothing verifies thatmin/max/count/avgare actually honored by fg. Since fg silently falls back tosumfor values it doesn't recognize, a wrongFG_COMBINERSentry would pass the suite while misbehaving in production. Onecountormaxparse case with sum-differing expected output would validate it cheaply. - The repo's established pattern for validation errors is
assertRaisesRegex(e.g.feature_test.py:748); matching"invalid combiner"would pin these tests to the new raise site rather than any unrelatedValueErrorescapingfg_json().
Code review — PR #673Carefully-researched set of fixes. I independently verified several of the PR's claims statically: the repo docs' overlap-method table matches Findings posted inline (6)
Minor notes (no inline)
Review coverage noteThis review ran five parallel areas: code quality, performance, test coverage, documentation accuracy, and security/compatibility. The first four completed and their noteworthy findings are the inline comments above; the dedicated security reviewer had not finished by the time this summary was posted, so the compatibility analysis in the first paragraph is my own (config persistence format, converter path, validation timing, and a repo-wide survey confirming no example/test config uses a combiner or method the new validation would reject). 🤖 Generated with Claude Code |
| @@ -74,6 +74,8 @@ def _fg_json(self) -> List[Dict[str, Any]]: | |||
| fg_cfg["separator"] = self.config.separator | |||
| if self.config.HasField("kv_delimiter"): | |||
| fg_cfg["kv_delimiter"] = self.config.kv_delimiter | |||
There was a problem hiding this comment.
The new proto comment adds a single-char contract ("only one char is allowed", matching docs feature.md:653), but the emission here is unchecked — a multi-char kv_delimiter flows into the fg json and gets silently mis-honored by fg, which is exactly the failure class this PR sets out to eliminate. Since combiner/method are already validated in _fg_json, a len(self.config.kv_delimiter) != 1 check here would be consistent.
Review follow-up: security / compatibility pass completedThe dedicated security/compatibility review (still running when my summary above was posted) has now finished; this updates the coverage note there. It confirmed my compatibility analysis (no binary proto serialization anywhere in Three additional points worth noting, one posted inline:
The reviewer also suggested hoisting the combiner/method validation to feature-construction/config-check time so 🤖 Generated with Claude Code |
kv_delimiter was declared as a float, so the ":" the docs describe could not be written at all and fg, which reads it as a one char string, would throw on the number. KvDotProduct declared a normalizer that the fg json never carried, so it was silently ignored. ComboFeature emitted the raw proto value_dim instead of the property, so a sequence combo feature with value_dim unset told fg to emit multi-value steps while the model assumed single values. combiner, overlap method and kv_delimiter are now rejected when fg can not honor them: fg silently falls back to sum for an unknown combiner, never accumulates gap_min/gap_max, keeps running with an unknown overlap method while every row returns the default value, and dies with a stack dump on a multi char kv_delimiter. The combiner is validated after the discrete and multi-value paths blank it out, because fg never receives it there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VkWBjeAQP8rcsutwL6rkfV
67306b3 to
b1fc940
Compare
2 participants
An audit of the fg docs and the fg C++ against TorchEasyRec's feature layer turned up config that fg never honors. Everything below was confirmed against the fg source and re-measured with pyfg 1.0.6.
Fixes
KvDotProduct.kv_delimiterwas typedfloat— the proto comment describes the default":", but afloatfield cannot hold it, and fg reads the key as a one-character string, so a JSON number throws. The field was unusable in every direction, including the EasyRec config converter, which assigns the source json value straight into it. Nowstring, with the[default = ":"]the siblingseparatorfield already has.KvDotProduct.normalizerwas declared but never emitted — fg parses it and applies it on every output path, but the feature's fg json never carried the key, so users got un-normalized dot products with no error.ComboFeatureemitted the raw protovalue_diminstead of the property — asequence_combo_featurewithvalue_dimunset emittedvalue_dim: 0while the inherited property returns 1, so fg produced multi-value sequence steps that the model never reduces (the embedding path gates the multi-value reduction onvalue_dim != 1).Validation added
fg is lenient in ways that turn a typo into a silently wrong feature, so the python layer now rejects the values it cannot honor:
combiner(LookupFeature / CombineFeature): fg maps any unrecognized combiner tosumwithout warning, andgap_min/gap_maxparse into enum values that are never accumulated — CombineFeature would train on a constant 0, while LookupFeature throws at construction. Allowed set issum / mean / avg / min / max / count. The check runs on the combiner that is actually emitted: a discrete or multi-value lookup blanks it out first (fg never receives it), so those configs are left alone, and an empty combiner is accepted for both features since fg treats it as sum.method(OverlapFeature): an unknown method logs an error but does not fail initialization, and every row then returns the default value, so the feature is a silent constant. Validated against the nine methods fg implements.kv_delimiter: fg rejects a multi-character delimiter withRuntimeError: new KVDotProduct failedand a 26-frame stack dump; the python check names the feature and the field instead.The proto comments and the two doc bullets that advertised
gap_min/gap_max(and listed 4 of the 9 overlap methods) are corrected here rather than in the docs-only follow-up, so the enforced set and the documented set land together.Upgrade note
Re-exporting an existing checkpoint now regenerates
fg.jsonwith theKvDotProduct.normalizerthat older tzrec silently dropped, so online feature values change relative to what that model was trained with. Worth a line in the release notes.Test Plan
kv_dot_product_test.py(stringkv_delimiterend to end plus alog10normalizer whose output is asserted, and a multi-char delimiter rejection),combo_feature_test.py(sequence combo withvalue_dimunset asserts 1 in the fg json), and negative cases inlookup_feature_test.py,combine_feature_test.py,overlap_feature_test.pyusingassertRaisesRegexon the raise site.overlap_feature_test.pyruns all nine methods through fg and asserts their distinct outputs, andlookup_feature_test.pygainsmin/max/countrows (1 / 3 / 2 on a map fg would otherwise sum to 4), so a wrong entry can no longer hide behind fg's silent sum fallback.raw_feature_test.pypins thatseparatorstays out of a scalar feature's fg json — an earlier revision of this PR emitted it, and measurement showed fg then abandons the row on a dim mismatch without filling it, returning uninitialized memory (different values every run) where master returns the default. That change is reverted; the doc rule ("separator only applies whenvalue_dim > 1") is in the docs PR.tzrec/features/*_test.pypass, plustzrec/datasets/data_parser_test.py.tzrec/tools/convert_easyrec_config_to_tzrec_config_test.pyfails identically on untouched master in this environment (TypeError: Descriptors cannot be created directlyfrom the vendored EasyRec protos), so it is unrelated to this change.pre-commit run --files <changed>andpyrefly checkclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01VkWBjeAQP8rcsutwL6rkfV