Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use deformable_detr kernel from the Hub #36853

Merged
merged 3 commits into from
Mar 21, 2025

Conversation

danieldk
Copy link
Member

What does this PR do?

Remove the deformable_detr kernel from kernels/ and use the pre-built kernel from the Hub instead.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@ArthurZucker @LysandreJik

Sorry, something went wrong.

@github-actions github-actions bot marked this pull request as draft March 20, 2025 12:10
Copy link

Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. When it is ready for review, please click the Ready for review button (at the bottom of the PR page).

@danieldk danieldk force-pushed the kernels-deformable-detr branch 5 times, most recently from d6bef46 to 05b6ee7 Compare March 20, 2025 12:49
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super super super nice! 🚀 let's just wait for @LysandreJik 's opinion on adding a new core dep!

setup.py Outdated
@@ -432,6 +434,7 @@ def run(self):
install_requires = [
deps["filelock"], # filesystem locks, e.g., to prevent parallel downloads
deps["huggingface-hub"],
deps["kernels"], # download kernels from the Hub
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the only place I'd want to wait @LysandreJik's input on! IMO this is the longterm plan for sure, wondering if we are gonna do that slowly (first optional soft dep) or not!

@HuggingFaceDocBuilderDev

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.

@danieldk danieldk marked this pull request as ready for review March 21, 2025 11:06
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's GO!!! 🤗


register_kernel_mapping(_KERNEL_MAPPING)

except ImportError:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect!

Remove the `deformable_detr` kernel from `kernels/` and use the
pre-built kernel from the Hub instead.
Also add it to `testing`, so that the kernel replacement gets tested
when using CUDA in CI.
@danieldk danieldk force-pushed the kernels-deformable-detr branch from 837ac4a to 96ae475 Compare March 21, 2025 11:37
@ArthurZucker ArthurZucker merged commit f94b0c5 into huggingface:main Mar 21, 2025
21 of 23 checks passed
@qubvel
Copy link
Member

qubvel commented Mar 21, 2025

Hey @danieldk! Super nice to see kernels integrated! Is there a way to disable the kernel path dynamically? RT-DETR's compile full graph and torch.export are broken now, they should go through the torch path.

@ArthurZucker
Copy link
Collaborator

Ah normally if cuda is not available it should fallback to noraml forwar + if not available should do nothing

@ArthurZucker
Copy link
Collaborator

(@qubvel export can be fast test no? sorry that it broke)

@qubvel
Copy link
Member

qubvel commented Mar 21, 2025

it's too slow, even though it's on a small model

@qubvel
Copy link
Member

qubvel commented Mar 21, 2025

we discussed with @danieldk in Slack, the idea is to have a way to control which path to go through, smth like

if self.disable_custom_kernels or is_torchdynamo_compiling():
    with use_kernel_mapping({}, inherit_mapping=False): # inherit_mapping would be a new argument
        output = self.attn(
            value,
            spatial_shapes,
            spatial_shapes_list,
            level_start_index,
            sampling_locations,
            attention_weights,
            self.im2col_step,
        )
else:
    ...

@ArthurZucker
Copy link
Collaborator

Or we register the use kernel with torch API to allow compile ?

@qubvel
Copy link
Member

qubvel commented Mar 24, 2025

Not aware of this but it can be an option, yes. Anyway it would be nice to be able to disable it manually in case there are any issues with the kernel + understand which path is executed (e.g. for RT-DETR to export for CoreML we need 5D tensors instead of 6D passed into the kernel, so I refactored pytorch path of deformable attention in this PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants