-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
New model support RTDETR #29077
New model support RTDETR #29077
Conversation
Looking good @SangbumChoi! Let us know when the PR is ready for review 🤗 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work adding this model!
Overall looking good - main comment is about the image processor preparing labels and the ordering of object definitions in the modeling file.
I've done a fairly high-level review this time and will do more in-depth over the next pass
@amyeroberts Hi amy! I think I can pass all the test in this week. Similar to other last PRs that I opened, I resolved conversation which is very simple or fixed as your comment. However, I did not resolved conversation which might need you to confirm or TO DOs. |
@amyeroberts Hi amy, Finally I have passed all the mandatory pass for this model. I think it is a good timing for to request 2nd review for the PR! Personally I think there are 3 things to check.
Also is it possible to have difference local pytest and ci test ? |
@amyeroberts Hi amy I added the test script you suggested! Maybe can you rerun the CI? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work adding this model!
@SangbumChoi Could you rebase to include the upstream changes on |
@amyeroberts Hi, amy. Thanks for the final review, it turns out that there was some issue beside rebase the current main branch. test_image_processor required appropriate dictionary in order to pass the test. So I changed at the upper commit! |
@SangbumChoi Great, thanks for handling that! Could you do a final empty commit for the slow tests?
|
@amyeroberts I have handled the typo + appropriate testing in |
@amyeroberts Hi amy, Could you do a final review? (I think for the rt_detr_resnet has no slow test so it is good to merge even though the CI fails?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SangbumChoi Thanks for all work on this!
cc @ydshieh This higlights a case where we might want to update the |
FYI) @ydshieh, @amyeroberts |
@SangbumChoi I think it's find to have the rt detr resnet under this model's folder, as @NielsRogge suggested. We just need to make sure our other tools can adapt to these kinds of patterns :) |
* fill out docs string in configuration https://github.com/huggingface/transformers/pull/29077/files/75dcd3a0e82cca36f12178b65bbd071ab7b25088#r1506391856 * reduce the input image size for the tests * remove the unappropriate tests * only 5 failes exists * make style * fill up missed architecture for object detection in docs * fix auto modeling * simple fix in missing import * major change including backbone refactor and objectdetectionoutput refactor * minor fix only 4 fails left * intermediate fix * revert __init__.py * revert __init__.py * make style * fixes in pr_docs * intermediate fix * make style * two fixes * pass doctest * only one fix left * intermediate commit * all fixed * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/models/rt_detr/convert_rt_detr_original_pytorch_checkpoint_to_pytorch.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * Update tests/models/rt_detr/test_modeling_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * function class above the model definition in dice_loss * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * simple fix * layernorm add config.layer_norm_eps * fix inputs_docstring * make style * simple fix * add custom coco loading test in image_processor * fix error in BaseModelOutput huggingface#29077 (comment) * simple typo * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * intermediate fix * fix with load_backbone format * remove unused configuration * 3 fix test left * make style * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: Sounak Dey <[email protected]> * change last_hidden_state to first index * all pass fix TO DO: minor update in comments * make fix-copies * remove deepcopy * pr_document fix * revert deepcopy due to the issue of unexpceted behavior in decoderlayer * add atol in final * add no_split_module * _no_split_modules = None * device transfer for model parallelism * minor fix * make fix-copies * fix typo * add test_image_processor with post_processing * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * add config in RTDETRPredictionHead * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * set lru_cache with max_size 32 * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * add lru_cache import and configuration change * change the order of definition * make fix-copies * add docs and change config error * revert strange make-fix * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * test pass * fix get_clones related and remove deepcopy * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * nit for paper section * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * rename denoising related parameters * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * check the image transformation logic * make style * make style * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * pe_encoding -> positional_encoding_temperature * remove TODO * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * remove eval_idx since transformer DETR is giving all decoder output * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * change variable name * make style and docs import update * Revert "Update src/transformers/models/rt_detr/image_processing_rt_detr.py" This reverts commit 74aa3e1. * fix typo * add postprocessing in docs * move import scipy to top * change varaible name * make fix-copies * remove eval_idx in test * move to after first sentence * update image_processor since box loss requires normalized one * change appropriate name to auxiliary_outputs * Update src/transformers/models/rt_detr/__init__.py Co-authored-by: NielsRogge <[email protected]> * Update src/transformers/models/rt_detr/__init__.py Co-authored-by: NielsRogge <[email protected]> * Update docs/source/en/model_doc/rt_detr.md Co-authored-by: NielsRogge <[email protected]> * Update docs/source/en/model_doc/rt_detr.md Co-authored-by: NielsRogge <[email protected]> * make style * remove panoptic related comments * make style * revert valid_processor_keys * fix aux related test * make style * change origination from config to backbone API * enable the dn_loss * fix test and conversion * renewal weight initialization * change initializer_range * make fix-up * fix the loss issue in the auxiliary output and denoising part * change weight loss to original RTDETR * fix in initialization * sync shape format of dn and aux * make style * stable fine-tuning and compatible conversion for resnet101 * make style * skip input_embed * change encoder related variable * enable converting rtdetr_r101 * add r101 related conversion code * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * Update docs/source/en/model_doc/rt_detr.md Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/__init__.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/__init__.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/models/rt_detr/image_processing_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * change name _shape to _reshape * Update src/transformers/__init__.py Co-authored-by: amyeroberts <[email protected]> * Update src/transformers/__init__.py Co-authored-by: amyeroberts <[email protected]> * maket style * make fix-copies * remove deprecated import * more fix * remove last_hidden_state for task-specific model * Revert "remove last_hidden_state for task-specific model" This reverts commit ccb7a34. * minore change in convert * remove print * make style and fix-copies * add custom rtdetr backbone for r18, r34 * remove print * change copied * add pad_size * make style * change layertype to optional to pass the CI * make style * add test in modeling_resnet_rt_detr * make fix-copies * skip tmp file test * fix comment * add docs * change to modeling_resnet file format * enabling resnet50 above * Update src/transformers/models/rt_detr/modeling_rt_detr.py Co-authored-by: Jason Wu <[email protected]> * enable all the rtdetr model :) * finish except CI * add RTDetrResNetBackbone * make fix-copies * fix TO DO: CI enable * make style * rename test * add docs * add special fix * revert resnet * Update src/transformers/models/rt_detr/modeling_rt_detr_resnet.py Co-authored-by: NielsRogge <[email protected]> * add more comment * remove swin comment * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: NielsRogge <[email protected]> * rename convert and add verify backbone * Update docs/source/en/_toctree.yml Co-authored-by: NielsRogge <[email protected]> * Update docs/source/en/model_doc/rt_detr.md Co-authored-by: NielsRogge <[email protected]> * Update docs/source/en/model_doc/rt_detr.md Co-authored-by: NielsRogge <[email protected]> * make style * requests for docs * more general test docs * general script docs * make fix-copies * final commit * Revert "Update src/transformers/models/rt_detr/configuration_rt_detr.py" This reverts commit d136225. * skip test_model_get_set_embeddings * remove target * add changes * make fix-copies * remove decoder_attention_mask * add load_backbone function for auto_backbone * remove comment * fix repo name * Update src/transformers/models/rt_detr/configuration_rt_detr.py Co-authored-by: amyeroberts <[email protected]> * final commit * remove unused downsample_in_bottleneck * new test for autobackbone * change to appropriate indices * test fix * fix dict in test_image_processor * fix test * [run-slow] rt_detr, rt_detr_resnet * change the slow test * [run-slow] rt_detr * [run-slow] rt_detr, rt_detr_resnet * make in to same cuda in CSPRepLayer * [run-slow] rt_detr, rt_detr_resnet --------- Co-authored-by: amyeroberts <[email protected]> Co-authored-by: Sounak Dey <[email protected]> Co-authored-by: NielsRogge <[email protected]> Co-authored-by: Jason Wu <[email protected]> Co-authored-by: ChoiSangBum <[email protected]>
Hi @amyeroberts. IMO, it's fine to just specify the folder name and run everything inside it. |
What does this PR do?
This is the new model for RTDETR that is complete version from #27247
There are several TO DOs
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
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.
@amyeroberts @NielsRogge