rbgirshick / yacs

YACS -- Yet Another Configuration System
Apache License 2.0
1.27k stars 89 forks source link

Freeze not working properly (or almost at all) #57

Open geniucos opened 1 year ago

geniucos commented 1 year ago

I was just trying to test out the freeze function and did something like this:

cfg = get_cfg_defaults()
cfg.freeze()
cfg.merge_from_file('input.yaml')

and it turns out merging from a file is let happen although the config node is allegedly frozen. I looked into the code and this makes sense since setattr (which is the only place that actually checks whether the object is immutable) is never called explicitly, but rather lines like this are used instead. Basically in the current, form frozen is only checked for one-time assignments (like cfg.lr=0.1).

There is another behavior which I doubt is intended: judging by the code, both merge_from_list and _merge_a_into_b descent into the tree structure without passing down the immutability information, so one could freeze the global (root) cfg but then be able to modify a grand child of it since the parent hasn't been frozen. Am I missing something? These two points deem freeze() in its current form almost useless.

Henning742 commented 1 year ago

I was reading the comments in the issue section and this might help. From https://github.com/rbgirshick/yacs/issues/56#issuecomment-1370172338:

YACS also allows you to enforce immutability so that once you've started your experiment/app and you handle user input, everything is frozen. This is very useful for repeatable experiments because you can log/store the config and you should be able to trust that the parameters weren't modified later on.

Basically, it's to avoid a user accidentally modifying some value in the config later on in the experiment. So if a user is already using merge_from_list, they would know that it's a yacs config and intend to modify it anyway. This might not be what freeze() is designed to prevent.