fix(data): align dynamic_bucket kwarg between Python stub and pybind - #1510
Open
Ace3Z wants to merge 1 commit into
Open
fix(data): align dynamic_bucket kwarg between Python stub and pybind#1510Ace3Z wants to merge 1 commit into
Ace3Z wants to merge 1 commit into
Conversation
meta-cla
Bot
added
the
CLA Signed
Ace3Z
added a commit
to Ace3Z/fairseq2
that referenced
this pull request
May 25, 2026
The Python stub `DataPipelineBuilder.filter` declared `predicate` while
the C++ pybind binding used `py::arg("fn")`, so calling
`pipeline.filter(predicate=...)` (the documented form) raised
`TypeError: incompatible function arguments`.
Same bug class as facebookresearch#1103 and the fix that landed in facebookresearch#1510 for
`dynamic_bucket`. Rename the pybind keyword from `fn` to `predicate`
so the runtime matches the documented Python signature. No internal
callsite uses the keyword form, so this is source-compatible for all
existing callers.
Add a regression test that exercises `filter(predicate=...)` and would
have caught the bug.
Follow-up to facebookresearch#1510.
The Python stub `DataPipelineBuilder.dynamic_bucket` declared `cost_fn`
while the C++ pybind binding used `py::arg("fn")`, so calling
`pipeline.dynamic_bucket(threshold=..., cost_fn=...)` (the documented
form) raised `TypeError: incompatible function arguments`.
Rename the pybind keyword from `fn` to `cost_fn` so the runtime matches
the documented Python signature and Sphinx-generated docs. No internal
callsite uses the keyword form, so this is source-compatible for all
existing callers.
Closes facebookresearch#1103
Ace3Z
force-pushed
the
fix/dynamic-bucket-cost-fn-kwarg-mismatch
branch
from
May 25, 2026 10:16
2fcebfa to
66babe3
Compare
Ace3Z
added a commit
to Ace3Z/fairseq2
that referenced
this pull request
May 25, 2026
fix(data): align filter() kwarg between Python stub and pybind
The Python stub `DataPipelineBuilder.filter` declared `predicate` while
the C++ pybind binding used `py::arg("fn")`, so calling
`pipeline.filter(predicate=...)` (the documented form) raised
`TypeError: incompatible function arguments`.
Same bug class as facebookresearch#1103 and the fix that landed in facebookresearch#1510 for
`dynamic_bucket`. Rename the pybind keyword from `fn` to `predicate`
so the runtime matches the documented Python signature. No internal
callsite uses the keyword form, so this is source-compatible for all
existing callers.
Follow-up to facebookresearch#1510.
Ace3Z
added a commit
to Ace3Z/fairseq2
that referenced
this pull request
May 25, 2026
The Python stub `DataPipelineBuilder.filter` declared `predicate` while
the C++ pybind binding used `py::arg("fn")`, so calling
`pipeline.filter(predicate=...)` (the documented form) raised
`TypeError: incompatible function arguments`.
Same bug class as facebookresearch#1103 and the fix that landed in facebookresearch#1510 for
`dynamic_bucket`. Rename the pybind keyword from `fn` to `predicate`
so the runtime matches the documented Python signature. No internal
callsite uses the keyword form, so this is source-compatible for all
existing callers.
Follow-up to facebookresearch#1510.
Author
|
Friendly ping on this one. It's a small binding fix for #1103 with a regression test. @cirquit @cbalioglu whenever you have a moment, would appreciate a look. |
Author
|
Friendly ping. @cbalioglu, would you have a moment to look at this? Tiny pybind/stub alignment fix. Happy to address any feedback. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1103.
Calling
pipeline.dynamic_bucket(threshold=..., cost_fn=...)(the form the Python stub atsrc/fairseq2/data/data_pipeline.py:238declares, and what Sphinx generates docs from) fails withTypeError: incompatible function arguments. The C++ pybind binding atnative/python/src/fairseq2n/bindings/data/data_pipeline.cc:503exposed the arg aspy::arg("fn").Renamed the pybind kwarg from
fntocost_fnso it matches the documented Python signature.cost_fnis also the C++ type alias name innative/src/fairseq2n/data/data_pipeline.h. Every internal caller (parquet table bucketing, existing tests) passes the cost function positionally, so this is source compatible.Added a regression test in
tests/unit/data/data_pipeline/test_dynamic_bucket.pythat callsdynamic_bucketwithcost_fn=as a kwarg. It fails on main with the exactTypeErrorfrom the issue and passes on this branch.I didn't rebuild fairseq2n from source locally (CUDA toolchain isn't on this box), so CI will give it its real exercise across the matrix. The change itself is a one character substring swap in an existing
py::arg()macro call, mechanically equivalent to its neighbors.Side note:
filterhas the same bug (predicatein the stub vspy::arg("fn")in the binding, around line 516). Sending that as a separate PR.