Repository navigation
Conversation
There was a problem hiding this comment.
Uh oh, could this actually be solved by changing
-
tofused_recurrent_gated_delta_rule(fla naming convention)
There was a problem hiding this comment.
Ok no noticed that its a different issue alltogether
|
I'm not sure I can follow 100%: Does this mean that the import path is different as per FLA versions or does this mean that the way the FLA package is installed may have changed the path - just would like this to be clarified so I have the correct overview In any case open to fix this! It should definitely try to work for all kind of installs / envs |
Thanks for checking. This is not caused by a different installation layout in my reproduction. I reproduced it with the same from fla.ops.gated_delta_rule import chunk_gated_delta_ruleThe issue is that The proposed fallback explicitly imports the mapped nested module when root traversal returns Different FLA versions may expose additional symbols at the package root, but the reproduced regression does not require an installation-path change. |
|
Wouldn't it make much more sense to adjust the behavior in |
Agreed, thanks for the suggestion. I moved the nested-module resolution into The common helper now preserves the existing package-root and attribute traversal behavior, then explicitly imports the nested parent module when needed. I also moved the regression coverage to I verified the full |
|
Hi @vasqu, the latest CI run has a single failure in an unrelated OwlViT numerical test:
The failed shard completed with 7,317 passed tests and this single failure. This PR only changes Could you please rerun the failed |
There was a problem hiding this comment.
Heya, I noticed that this is a versioning issue no? I can repro on fla == 0.5.0 but on the most recent fla version 0.5.2, this works without this workaround.
Tbh, this seems more like an package structure issue on FLA that was only recently solved. I'd really like to avoid making this change here as the package itself should be a properly nestable module and not only work via import module on the full path.
Tl;dr: Bump fla please, it's more of a package structure issue on fla side (solved on latest versions)
Thanks for pointing this out. I retested this in a clean environment with both On the current unmodified Transformers main, the existing resolver correctly selects:
I also ran the Qwen3.5 gated-delta wrapper on CUDA and verified that forward, backward, and all input gradients complete with finite values. Under the same unmodified Transformers code, FLA 0.5.0 still selects the PyTorch fallback. This confirms that the behavior was caused by the older FLA package structure and is fixed in FLA 0.5.2. You're right that the appropriate fix is to bump FLA downstream rather than change the generic Transformers resolver. Thank you for the clarification. I'll close this PR. |
|
Thanks @imvladikon for pointing this out. I reproduced the core-only case and reopened both this PR and #48148. On current Transformers
As a result, The earlier 0.5.2 verification had both I agree that changing
The CPU prototype selects both FLA kernels correctly, preserves package-root resolution, and retains the missing-kernel fallback. @vasqu, would this narrower change inside |
|
It's easier for me to decide when I can see an implementation :D did the earlier comments get deleted? Anyways, this is super annoying and makes FLA package structure really unconventional. Maybe we can just adjust the original fallback fn and make a total import as second option (only for whitelisted packages like FLA) |
|
@vasqu I apologize. Yes, I deleted it because I start thinking that it's indeed a problem with the fla packaging, and not sure about a solution |
|
No worries, I'm also a bit baffled at their package structure tbh 😢 I mean we could allow full imports but it's also a matter of, is it really intended to only have core installed for example |
Yes, The official FLA documentation describes the distribution split as follows:
References:
Therefore, the core-only environment is an intentional package configuration rather than a partially installed full package. It is also sufficient for the two kernel paths reproduced here: I will update the implementation so that:
I will push the implementation and verification results to this PR for review. |
|
Sounds good! This FLA structure really is unconventional tho... Anyways I think it's the right approach to treat pkgs like FLA a bit more special then |
|
Implemented and pushed in The updated implementation follows the agreed scope:
The PR now changes only:
Verification completed:
One CI job,
No test assertion or traceback related to this PR was reported. Could you please rerun that failed job? Ready for review. |
vasqu
left a comment
There was a problem hiding this comment.
Got some smaller comments but I think it mightve been overkill with the whitelist (sorry)
| if implementation is None and package in _KERNELS_EXPLICIT_IMPORT_PACKAGES and internal_path is not None: | ||
| internal_module = importlib.import_module(f"{package}.{internal_path}") | ||
| implementation = getattr(internal_module, func_name, None) |
There was a problem hiding this comment.
Would it make sense to change the declarations a bit so
full_path->full_func_pathfull_module_path
| if implementation is None and package in _KERNELS_EXPLICIT_IMPORT_PACKAGES and internal_path is not None: | |
| internal_module = importlib.import_module(f"{package}.{internal_path}") | |
| implementation = getattr(internal_module, func_name, None) | |
| if implementation is None and internal_path is not None: | |
| module = importlib.import_module(full_module_path) | |
| implementation = getattr(module, func_name, None) |
| "selective_scan_fn": "ops.selective_scan_interface", | ||
| } | ||
|
|
||
| _KERNELS_EXPLICIT_IMPORT_PACKAGES = {"fla"} |
There was a problem hiding this comment.
At second thought it might be a bit overkill
| @@ -56,6 +55,113 @@ | |||
| import transformers.integrations.hub_kernels as hub_kernels_pkg | |||
|
|
|||
|
|
|||
| class TestKernelFallback(TestCasePlus): | |||
There was a problem hiding this comment.
Let's add the tests under the utils class no?
|
Addressed the latest review comments in
Verification completed:
The updated commit is ready for review. |
vasqu
left a comment
There was a problem hiding this comment.
Thanks 🫡 really nitpicky but wanna make sure this goes well as its a special case
| module = importlib.import_module(package) | ||
| implementation = resolve_internal_import(module, full_path) | ||
| implementation = resolve_internal_import(module, full_func_path) | ||
| if implementation is None and internal_path is not None: |
There was a problem hiding this comment.
| if implementation is None and internal_path is not None: | |
| if implementation is None and full_module_path != package: |
nit
| module = importlib.import_module(package) | ||
| implementation = resolve_internal_import(module, full_path) | ||
| implementation = resolve_internal_import(module, full_func_path) | ||
| if implementation is None and internal_path is not None: |
There was a problem hiding this comment.
Let's add a small comment here and mention fla as example
| @@ -419,6 +418,110 @@ def test_enable_hub_kernels(self): | |||
|
|
|||
| @require_kernels | |||
| class TestKernelUtilities(TestCasePlus): | |||
| def test_fallback_resolves_function_from_package_root(self): | |||
There was a problem hiding this comment.
2 smaller nits:
- Let's add new tests at the end pls
- Could we parametrize these tests a bit? Unsure if we really need separate tests for all of those
cdf94e6 to
4c13bec
Compare
|
Thanks @vasqu! I addressed all of the review comments in
I also rebuilt the branch on top of the latest Validation:
Ready for another review. |
vasqu
left a comment
There was a problem hiding this comment.
Just some very last comments but we should get it merged after that, thanks a lot 🤗
| ) | ||
| def test_fallback_imports_nested_module( | ||
| self, | ||
| _case_name, |
There was a problem hiding this comment.
| _case_name, | |
| case_name, |
lets avoid it please
| internal_path="ops.kernel", | ||
| )(torch_function) | ||
|
|
||
| self.assertEqual(wrapped(), "torch") |
There was a problem hiding this comment.
Let's add small comments / docstrings as well just for the general what are we trying here but nice tests otherwise
|
Addressed the final review comments in
Validation:
Thanks again for the review! @vasqu |
|
Merging now, thanks for sticking through @DimensionSTP and also @imvladikon for reopening this issue otherwise we wouldve ignored it for who knows how long |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
CI recapDashboard: View test results in Grafana |
* fix: resolve nested optional kernel imports * test: clarify nested kernel fallback coverage
What does this PR do?
This fixes optional FLA kernel resolution when only
fla-coreis installed.In the core-only layout,
import fladoes not necessarily attachfla.opsto the package object. The existing package-root traversal may therefore returnNoneeven though these nested kernels are directly importable:fla.ops.gated_delta_rule.chunk_gated_delta_rulefla.ops.kda.chunk_kdaThe existing package-root resolution remains unchanged. If it returns
None, this PR explicitly imports the mapped nested module as a second option.This behavior is restricted to an explicit package whitelist containing only
fla. Non-whitelisted packages retain the existing behavior, and failed nested imports continue to use the Torch fallback.Fixes #48148
Verification
tests/kernels/test_kernels.py::TestKernelFallback: 4 passedtests/utils/test_import_utils.py: 161 passedgit diff --check: passedfla-core==0.5.2:Code Agent Policy
The Transformers repo is currently being overwhelmed by a large number of PRs and issue comments written by
code agents. These often are low-quality, or fix extremely minor issues that occur rarely or never in practice.
As a result, we're instituting a rule that first-time contributors should not use code agents to submit PRs or issues.
We'd also ask autonomous "OpenClaw"-like agents not to open any PRs or issues.
Issues/PRs from first-time contributors that violate this rule will probably just be closed without review, and we
might block you, especially if you open more than one or appear to be deliberately ignoring this. We especially do not
want new contributors to jump in on random issues to contribute an agent-written fix. This creates lots of noise
for reviewers and other users and will almost certainly get you blocked.
For more information, please read
CONTRIBUTING.md.Before submitting
Pull Request checks?
Who can review?
@vasqu @drbh