[Fix] Fix dump method of Config #1837
Conversation
dump should not depend on how config was loaded only where it is dumped changed checks for type to the given filename instead of the source filename
|
|
|
oops thanks for the review, I didn’t think this through I’ll give it another try... |
it might be more convoluted than it needs to be but the formatting depends on the extension of the argument or the extension of the self.filename
|
When there is no input file argument, it does not save the config in a file but returns a string. I am not sure this is the desired behavior but the format of this string depends on self.filename (format of the string matching source format). I personally don’t think this is the best choice but I think this is the closest behavior to the previous version. (To me the dump string format should always be the same regardless of the source of the config.) |
Besides, it seems the original implementation will return python-format pretty_text regardless of the file format of |
|
@jmercat Hi.
Hi~ After discussing with my partners, your code implementation makes more sense and will not generate BC breaking risk. But there exists a small bug, If |
|
Thanks for your reviews I’ll make these changes (name of the test and bug fix) tomorrow or Friday. |
…to test_dump_from_dict
|
@jmercat Thanks for your docstring. But it seems the format is a little weird. Have you committed your code with pre-commit(https://github.com/open-mmlab/mmcv/blob/master/CONTRIBUTING.md)? Besides, |
|
Hi @jmercat , is there any progress? |
|
I added other email addresses to my github account. I think it should solve the CLA signing problem. |
|
This is a good learning experience for me but I think that for such a small change, the formatting, cla, and all add an overhead that is larger than the contribution. Next time I’ll probably just write an issue, propose a code change there and let a bigger contributor implement it. It might even be faster for you, then you won’t need all these discussions as we had here. You might want to warn about this in the contribution guidelines. |
Hi @jmercat, thanks for your contribution. You are welcome to open an issue to report anything. |
zhouzaida
changed the title
* config dump fix dump should not depend on how config was loaded only where it is dumped changed checks for type to the given filename instead of the source filename * defined test_fromdict and fixed dump function it might be more convoluted than it needs to be but the formatting depends on the extension of the argument or the extension of the self.filename * config dump defaults to returning pretty_text, test_fromdict renamed to test_dump_from_dict * some reformatting in docstrings * refine unittest * fix unit test as comment * Refine docstring * minor refinement * import mmcv * fix lint Co-authored-by: HAOCHENYE <21724054@zju.edu.cn> Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>
Motivation:
When loading a config from a dict, the dump function does not work. Dump should not depend on how config was loaded but only where it is dumped.
Modification:
Changed checks for type in dump. It used to check the config source filename and I changed it to the destination filename. When there is no input file argument, it does not save the config in a file but returns a string. I am not sure this is the desired behavior but the format of this string depends on self.filename (format matching source format). I personally don’t think this is the best choice but I think this is the closest behavior to the previous version.
Use cases (Optional)
It would offer the ability for a config loaded from a dict to be dumped.