Skip to content
New issue

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

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

Already on GitHub? # to your account

[Fix] Fix potential bug about output_config overwrite #463

Merged
merged 3 commits into from
Dec 19, 2020

Conversation

CarolineCheng233
Copy link
Contributor

@CarolineCheng233 CarolineCheng233 commented Dec 19, 2020

This PR fixes potential bug when users do not assign parser args. For example, if a user does not assign args.out

output_config = dict(out=f'{work_dir}/results.json', output_format='json')
args.out = None
output_config = Config._merge_a_into_b(dict(out=args.out), output_config)

then, output_config in config files will be overwritten.

@CLAassistant
Copy link

CLAassistant commented Dec 19, 2020

CLA assistant check
All committers have signed the CLA.

@CarolineCheng233 CarolineCheng233 changed the title [Fix] Fix potential bug when users do not assign parser args [Fix] Fix potential bug about output_config overwrite Dec 19, 2020
@codecov
Copy link

codecov bot commented Dec 19, 2020

Codecov Report

Merging #463 (b13224d) into master (137351c) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
+ Coverage   84.67%   84.71%   +0.03%     
==========================================
  Files         118      118              
  Lines        8347     8347              
  Branches     1366     1366              
==========================================
+ Hits         7068     7071       +3     
+ Misses        932      931       -1     
+ Partials      347      345       -2     
Flag Coverage Δ
unittests 84.70% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmaction/core/evaluation/accuracy.py 94.52% <0.00%> (+1.36%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 137351c...b13224d. Read the comment docs.

@innerlee
Copy link
Contributor

Thanks! This bug was introduced in #310, in which we replaced our own merge function with mmcv's.

In our version of merge, we treat None as transparent, and would not overwrite things.
It was my oversight that, I wrongly thought mmcv's merge would respect this semantic.

This has caused many problems. Now I'm thinking whether we should bring our merge function back. mmcv's functions sometimes are less thoughtful :/

@innerlee innerlee merged commit 30ff6b2 into open-mmlab:master Dec 19, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants