Skip to content

Conversation

Zeyi-Lin
Copy link
Member

@Zeyi-Lin Zeyi-Lin commented May 30, 2024

Description

  1. 较大地改进了config类,以应对更多可能出现bug的情况
  2. 增加了对config的单元测试

Close: #594


其中对于在swanlab.init前定义swanlab.config的逻辑:

  1. 我们支持在swanlab.init前,使用swanlab.config.updateswanlab.config.set等方式设置key, value,swanlab.init的时候config会被更新;
  2. 如果直接用swanlab.config={...}的方式,相当于把SwanLabConfig类变成了一个dict,那么swanlab.init的时候config并不会被swanlab.config的内容更新,且swanlab.config失去了原有的诸多效果,但run.config并不受影响,代码的运行也不受影响
  3. 第2点会在文档中提起
  4. swanlab.finish()之后,config将被清空

@Zeyi-Lin Zeyi-Lin requested a review from SAKURA-CAT May 30, 2024 14:25
@Zeyi-Lin Zeyi-Lin self-assigned this May 30, 2024
@Zeyi-Lin Zeyi-Lin added this to the v0.3.x milestone May 30, 2024
@SAKURA-CAT
Copy link
Member

然后有几个问题:

  1. 既然测试的是config这个类,那么就应该单独测试config,而不是通过SwanLabRun初始化config,否则代码会十分冗余;即使要测试通过SwanLabRun初始化config的正确性,也只需测试一个函数即可
  2. 传入的config_data应该可以被Json序列化,这意味着如果传入类似math.nan等数据,应该报错或者warning等操作
  3. 如果传入的config_data中包含其他的自定义类(比如,omegaconf),应该判断其是否定义了__dict__方法或是否继承自抽象的Mapping,因此__check_config函数需要修改,不应该单纯判断是否为类argparse.Namespace实力

@SAKURA-CAT
Copy link
Member

相关测试我添加在了测试脚本里,但是这只是一个示例,测试脚本可能还需要更改:

    def test_insert_class(self):
        """
        测试插入类
        """
        from collections.abc import MutableMapping

        class Test(MutableMapping):
            def __init__(self, a, b):
                self.data = {"a": a, "b": b}

            def __setitem__(self, __key, __value):
                self.data[__key] = __value

            def __delitem__(self, __key):
                del self.data[__key]

            def __getitem__(self, __key):
                return self.data.get(__key, None)

            def __len__(self):
                return len(self.data)

            def __iter__(self):
                return iter(self.data)

        config_data = {
            "a": 1,
            "b": "mnist",
            "c/d": [1, 2, 3],
            "e/f/h": {"a": 1, "b": {"c": 2}},
            "test": Test(1, 2)
        }

        run = SwanLabRun(run_config=config_data)
        assert run.config.test.a == 1
        assert run.config.test.b == 2

    def test_not_json_serializable(self):
        """
        测试不可json化的数据
        """
        import math, json
        config_data = {
            "a": 1,
            "b": "mnist",
            "c/d": [1, 2, 3],
            "e/f/h": {"a": 1, "b": {"c": 2}},
            "test": math.nan
        }
        run = SwanLabRun(run_config=config_data)
        json_data = json.dumps(SwanLabRun.config)

@SAKURA-CAT
Copy link
Member

其他大致上应该没什么问题,主要是config.py可能需要重写一下

@SAKURA-CAT SAKURA-CAT changed the base branch from main to feat/cloud-only-mode June 2, 2024 10:03
@Zeyi-Lin
Copy link
Member Author

Zeyi-Lin commented Jun 2, 2024

对于config的value是一个类实例的情况,目前统一转成str(类的类型太多,实例化难以统一),所以插入类的操作暂时实现不了。

@SAKURA-CAT
Copy link
Member

对于config的value是一个类实例的情况,目前统一转成str(类的类型太多,实例化难以统一),所以插入类的操作暂时实现不了。

那这样似乎没法解决omegaconfig的问题?

@Zeyi-Lin
Copy link
Member Author

Zeyi-Lin commented Jun 2, 2024

对于config的value是一个类实例的情况,目前统一转成str(类的类型太多,实例化难以统一),所以插入类的操作暂时实现不了。

那这样似乎没法解决omegaconfig的问题?

omegaconfig就特殊处理就可以,当前代码中已经适配(之前的版本已经适配了omegaconf,用json_serializable函数就可以,这次是更细致的适配)

@SAKURA-CAT
Copy link
Member

SAKURA-CAT commented Jun 2, 2024

对于config的value是一个类实例的情况,目前统一转成str(类的类型太多,实例化难以统一),所以插入类的操作暂时实现不了。

不用这么复杂,你看omegaconfig的源码就知道返回值继承自MutableMapping ,并且vars函数能直接fmt此类,原因是它实现了__dict__属性

@Zeyi-Lin
Copy link
Member Author

Zeyi-Lin commented Jun 2, 2024

对于config的value是一个类实例的情况,目前统一转成str(类的类型太多,实例化难以统一),所以插入类的操作暂时实现不了。

不用这么复杂,你看omegaconfig的源码就知道返回值继承自MutableMapping ,并且vars函数能直接fmt此类,原因是它实现了__dict__属性

不是,是因为omegaconfig直接转成dict的话,和目标效果不一致,所以用了omegaconf.OmegaConf.to_container(data, resolve=True, throw_on_missing=True)

@Zeyi-Lin
Copy link
Member Author

Zeyi-Lin commented Jun 2, 2024

可以针对MutableMapping类型做特定优化

@SAKURA-CAT
Copy link
Member

SAKURA-CAT commented Jun 2, 2024

第三方处理可以有,但是应该有一个更加通用的处理规则:

  1. 如果是第三方,应用第三方处理
  2. 如果实例实现了__dict__,则使用__dict__json.dumps解析此属性即可
  3. 如果实例继承自MutableMapping,则json.dumps可以直接解析此实例

2和3属于通用方法,但是不排除外界使用写出bug,所以需做好一些提示

@SAKURA-CAT
Copy link
Member

因此最终保存在SwanLabConfig里的是一个对象,只有在执行相关魔术方法的时候才会进行json序列化

需要注意的是假如传入了一个test对象:

# 此时会触发__setitem__,因此会触发save事件
config["test"] = test
# 此时会触发__setattr__,因此会触发save事件
config.test = test
# 此时不会触发save事件
config["test"].a = 1
config.test.a = 1
# 如果此情况需要触发save事件,应该通过此方法手动触发
config.save()

上述情况需要在相关文档中注明

@Zeyi-Lin Zeyi-Lin merged commit ccc4133 into feat/cloud-only-mode Jun 2, 2024
Zeyi-Lin added a commit that referenced this pull request Jun 4, 2024
* Feat/improve_config (#595)

* Create pytest_config.py

* update config location

* config clean

* update more pytest

* add omegaconf pytest

* Update pytest_config.py

* pytest add get_config

* Update pytest_config.py

* update run.config.clean

* update some details

* fmt import

* update requriement

* rename config

* change type hit

* Update pytest_config.py

* update clean

* update config

* Update config.py

* update check_config and save

* __save__ to save

---------

Co-authored-by: KAAANG <[email protected]>

* update pytest and del init_need

---------

Co-authored-by: KAAANG <[email protected]>
@SAKURA-CAT SAKURA-CAT deleted the feat/improve-config branch June 4, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[REQUEST] 优化Config-改进逻辑,添加相关单元测试

2 participants