-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Improve documentation concerning the new config files #3225
Comments
I think this is actually a bug 🐛 @ppwwyyxx Could you confirm whether this is a bug or expected behavior? I noticed you authored the last commit in the |
This is actually a bug. I had similar issues but when I added the required 'new-baseline' config manually into model_zoo.py file, this was solved. |
I created a pull request to add the mapping from config to model checkpoint for the new baselines. However, I do not think that solves this particular issue. Even though the transition from YAML to PY configs is easy, the documentation could (and should) still be updated to reflect these changes. |
Summary: Related to #3225, but does **not** solve it. # Overview of changes FAIR recently published new [Mask R-CNN baselines](https://ai.facebook.com/blog/advancing-computer-vision-research-with-new-detectron2-mask-r-cnn-baselines/). The new config files have not yet been mapped to the corresponding model checkpoint files in the `model_zoo._ModelZooUrls` class. This was a relatively easy fix that simply follows the table given in the [MODEL_ZOO table](https://github.com/facebookresearch/detectron2/blob/master/MODEL_ZOO.md#new-baselines-using-large-scale-jitter-and-longer-training-schedule). If we run the following code snippet below: ```python from detectron2 import model_zoo model = model_zoo.get("new_baselines/mask_rcnn_regnetx_4gf_dds_FPN_400ep_LSJ.py", trained=True) ``` This used to give: ``` RuntimeError: new_baselines/mask_rcnn_regnetx_4gf_dds_FPN_400ep_LSJ not available in Model Zoo! ``` With these new changes, we get what we would expect and actually download and load the model: ``` model_final_f1362d.pkl: 173MB [01:13, 2.37MB/s] The checkpoint state_dict contains keys that are not used by the model: proposal_generator.anchor_generator.cell_anchors.{0, 1, 2, 3, 4} ``` I am not entirely sure whether the warning is expected or something that requires fixing. # Additional proposal I personally am not a big fan of the naming. "new_baslines" seems too general to me and deviates from the already established naming conventions. This makes it less intuitive and not very robust to future updates. I would suggest moving the configs to `configs/COCO-InstanceSegmentation`. Similarly, I would then also move the checkpoint files accordingly to `https://dl.fbaipublicfiles.com/detectron2/COCO-InstanceSegmentation/`. I did not implement these changes yet because I cannot change where the model files are stored and because I can imagine there to be an internal motivation not to change the naming anymore. Pull Request resolved: #3228 Reviewed By: vaibhava0 Differential Revision: D29590936 Pulled By: ppwwyyxx fbshipit-source-id: ffebeff845acfa2d9beb6d0e5472ff8670fbb457
Thanks for the help so far! I indeed do think the documentation should still be updated. I find it especially confusing that the two config types seem to use different keys. For example |
@ppwwyyxx I have been trying to compose a minimal script to understand the usage of the new cfg = get_config('new_baselines/mask_rcnn_regnety_4gf_dds_FPN_400ep_LSJ.py', True)
model = instantiate(cfg.model)
dataset = instantiate(cfg.dataloader.train)
optimizer = instantiate(cfg.optimizer)
trainer = SimpleTrainer(model, dataset, optimizer)
trainer.train(0, 700000) I think this is the absolute bare minimum. It does not do any evaluation and does not even save the model. However, this already causes issues that make me suspect that the new config files are not fully supported yet. For example; There also does not yet seems to be a |
https://github.com/facebookresearch/detectron2/blob/master/tools/lazyconfig_train_net.py is the training script to use these configs. We'll add documentation soon. |
Awesome! Thank you for the quick response. I completely overlooked that file. Sorry for the inconvenience. |
@orbiskcw Can you share your training code please |
It's not worth sharing. The provided training script that was just mentioned contains all the information you need. I just updated some configurations so that we can train it on our own data. I added code for the custom dataset and used
sharing my script will not help you as I barely changed anything and what I did change is application specific |
i am new with code thats why i need it |
@ppwwyyxx Saving and loading a config file that had a lambda does not seem to work yet. For example: cfg = get_config('new_baselines/mask_rcnn_regnety_4gf_dds_FPN_400ep_LSJ.py')
LazyConfig.save(cfg, "tmp.yaml")
cfg = LazyConfig.load("tmp.yaml")
model = instantiate(cfg.model)
print(model) Gives:
Manually removing the lambdas from the generated Generally, however, I am not sure whether and how lambdas could (and should) be supported with the lazy config format. What are your thoughts on this? |
Lambda should not be saved in a yaml - I don't think there is a reasonable way to do that. |
Summary: fix #3225 Pull Request resolved: fairinternal/detectron2#553 Reviewed By: wat3rBro Differential Revision: D29731838 Pulled By: ppwwyyxx fbshipit-source-id: 69ed27681747cf00a2fa682c245369c3d1ee8945
We added documentation in https://detectron2.readthedocs.io/en/latest/tutorials/lazyconfigs.html. Feel free to suggest improvements or missing information. |
Thank you for this! It reads quite nicely. There is two points I would like to add to the documentation:
|
I agree with the comments above. An additional question would be whether there are any concrete plans to further expand the support for these new config files throughout the repository? For example, by adding a new DefaultTrainer-like class for lazy configs. If there were to be some sort of overview or roadmap of changes planned / needed, I would happily pick up some of those tasks! |
I agree we should explain more on this topic. Among your solutions: (1) and (3) are actually almost the same. They share the same limitation that the config object can be made unserializable if users choose to use complex objects. I'm not so sure on how (2) can be done.
As you see there isn't a simple 1-1 mapping. Because config follows the code directly, the new config is most suitable for users with enough expertise so that they can directly work with arguments in the code. But still, I think we can list a few examples on the commonly used options.
|
Hey guys, I am training a simple mask R CNN model for instance segmentation. I found that the new-baselines pre-trained weights offer significantly better accuracy but I am not able to use it as such. I went through the documentation of @ppwwyyxx and Cheers, |
Hey @VishalBalaji321, it seems like the # Overrides the maximum number of iterations by 100 and the learning rate by 0.1
python lazyconfig_train_net.py --config-file ... train.max_iter=100 optimizer.lr=0.1 Of course, you coud also do this in code. Similarly to what is done here. # Overrides the maximum number of iterations by 100 and the learning rate by 0.1
cfg.train.max_iter = 100
cfg.optimizer.lr = 0.1 Without any concrete questions, I can't really help you more than that. I'm afraid that, like @ppwwyyxx said as well, this new config requires you to understand the code a bit better than before. But I have been working with it for a little while now and I actually have been enjoying it more and more. It's quite powerful! |
Thanks a lot for the previous reply @orbiskcw , it did help me a lot. Now I am facing issues with 'SyncBN' normalization. Could you pls explain how to change it to just 'BN' for single GPU? (For Info: I am planning on using newbaselines/R101_FPN_400ep model |
Happy to have helped. I am pretty sure that the change depends on the model you are using. For me, it was # Using the python syntax here, but as I have shown earlier
# it looks similar if you provide these overrides using the CLI
cfg.model.backbone.bottom_up.norm = "BN"
cfg.model.backbone.norm = "BN" To help you find the right keys, I would recommend doing one of two things:
Note that because the lazy configs closely follow the code, not all configurations you are looking for will be present in the configs or easy to find with either of these two options. I'm afraid there is no other solution than to just bite the bullet and dig in the Detectron2 code! I think you will find it is nicely structured and quite easy to comprehend once you get going! 😉 |
@VishalBalaji321 In case you have not seen this yet, the documentation was updated yesterday: https://detectron2.readthedocs.io/en/latest/tutorials/lazyconfigs.html It explains a lot of the things I tried to explain to you as well, but better! 😉 @ppwwyyxx Great work by you and your colleagues. It feels a lot more complete now. Nothing comes to mind anymore that could be added. |
@ppwwyyxx One more question that recently came up while working with the detectron2 repo: What is the best way to extend a config from the model zoo? I expected something like this to work: # In /path/to/custom_config.py
from detectron2.model_zoo.configs.new_baselines.mask_rcnn_regnetx_4gf_dds_FPN_400ep_LSJ import (
dataloader,
lr_multiplier,
model,
optimizer,
train,
)
train.max_iter = 10 But if I then try to load the new config with: LazyConfig.load('/path/to/custom_config.py') I get:
Could you confirm whether this is expected behavior or a bug? Could you clarify if it is possible to do this and if so, what the conditions / requirements are? |
Something like this does work, but feels very cumbersome: config = get_config('new_baselines/mask_rcnn_regnetx_4gf_dds_FPN_400ep_LSJ.py')
train = config.train
optimizer = config.optimizer
model = config.model
dataloader = config.dataloader
lr_multiplier = config.lr_multiplier
del config
train.max_iter = 10 |
@ppwwyyxx Do you see the chance to still respond to the above question? |
The failure is currently expected because when doing However it would be nice to enable detectron2/detectron2/config/lazy.py Lines 124 to 130 in fe4d35b
which currently only handles relative import. |
Clear! I have taken a look at the method you referenced, but this is some Python wizardry I am not really familiar with and I wouldn't know where to start to support absolute imports. I might take a closer look at it once I have a bit more time, but for now I will stick to the workaround. Thank you for your reply! 👍 |
The value of this config should be a config object, but not the actual data itself. So you'd want
See examples in detectron2/configs/common/data/coco.py Lines 15 to 16 in 6886f85
|
Thank You for the very quick reply. Now a quick question that might be easy. I just want to know if there are any specific functions that I can use. How can I predict on single images with my saved model? Thank You EDIT:
But I get the following error:
I don't know if this is the right way to do the predictions. But if it is, I don't exactly understand how to resolve this error. |
As documented in https://detectron2.readthedocs.io/en/latest/tutorials/models.html#model-input-format the image of model's input has to be in (C, H, W) format while in your code it's (H, W, C). DefaultPredictor would need a patch like #3755 to work with new configs. |
If anyone is looking for a solution for doing inference on single images remember to:
where d is |
Hello. Thank you in advances. |
Same problem |
Hi, I have carefully read the docs and the tutorial, but is there an equivalent of cfg.INPUT.MASK_FORMAT = 'bitmask' for the new baselines? I could use it with previous pipelines but with the new one I cant train on my custom dataset @ppwwyyxx |
@rezat96 cfg.dataloader.train.mapper['instance_mask_format'] = 'bitmask' |
Is there a way to convert new configs to old configs that will work with DefaultPredictor? |
📚 Documentation Improvements
In short
Concerning: https://detectron2.readthedocs.io/en/latest/tutorials/configs.html
Problem: Documentation does not seem to have been updated to reflect the new config files (
.py
rather than.yaml
)Solution: Update the documentation
Problem description
FAIR recently published new Mask R-CNN baselines and this was my first introduction to the new config file that no longer relies on YAML files but on 'raw'
.py
files. I am trying to load the new baselines using the config files mentioned in theMODEL_ZOO
(see this table). For example:This gives
I have installed Detectron2 using the installation instructions. When looking up the documentation on configs, it seems that this has not been updated to reflect the new configs and still solely mentions YAML files.
Proposed solution
It could be that the
CONFIG_PATH_TO_URL_SUFFIX
dictionary in_ModelZooUrls
class still has to be updated and that this is actually a bug (see here), but I find it hard to estimate wheter this is meant behavior (i.e. the new config file should be loaded differently) or a bug due to my limited understanding of the new config files. Either way, I therefore feel like the documentation on readthedocs should be updated to reflect the change from.yaml
to.py
.The text was updated successfully, but these errors were encountered: