Skip to content

Conversation

@shade233
Copy link
Contributor

@shade233 shade233 commented Aug 7, 2025

Fixed conflicts caused by multiple instances of argparser;
Fixed incorrect usage of the bool type in argparser;
Improved the dataset selection mechanism within argparser;

Additionally, it is not recommended to pass parameters via argparser; instead, consider using a configuration file to specify them. Also, please avoid mixing naming conventions with or without underscores in the code, such as 'lolv2_syn' and 'lol_v2_syn'. One consistent style should be used throughout.

Fixed conflicts caused by multiple instances of argparser;
Fixed incorrect usage of the bool type in argparser;
Improved the dataset selection mechanism within argparser;

Additionally, it is not recommended to pass parameters via argparser; instead, consider using a configuration file to specify them.
Also, please avoid mixing naming conventions with or without underscores in the code, such as 'lolv2_syn' and 'lol_v2_syn'. One consistent style should be used throughout.
@Fediory
Copy link
Owner

Fediory commented Sep 8, 2025

Sorry for the late reply.
Have you thoroughly tested the code you modified?
If everything is in order, is there anything in my README that needs to be updated?

@shade233
Copy link
Contributor Author

shade233 commented Sep 9, 2025

修改主要集中在参数解析和数据集选择逻辑两部分:

1. 参数解析

原始代码中,所有 bool 类型参数使用 type=bool,这会导致无论用户输入什么非空字符串(包括 'False'),结果都被解析为 True。

修改后引入了 _str2bool 方法,用户可以通过 --start_warmup true/false 的方式正确传参。如果用户不输入参数,则使用默认值。

另一种可选方案是改为使用开关参数(如 --start_warmup 表示开启),但这会涉及到参数命名的大幅调整,可能会影响和旧代码的兼容性,所以目前没有采用。

2. 避免多个 argparser 冲突

原来部分依赖文件(如 eval.py)在被 train.py 导入时也会初始化自己的 argparser,造成重复解析和参数冲突。

修改后这些文件只在作为独立脚本运行时才会解析参数,从而避免冲突。

3. 数据集选择机制

原始代码通过多个布尔开关参数(--lol_v1、--lolv2_real 等)来指定数据集,这容易导致用户同时设置多个数据集,逻辑不直观。

修改后统一为 --dataset xxx 的形式(如 --dataset lol_v1),并在代码中相应调整了判断逻辑。这样可以避免用户误操作,同时使用方式更直观。

这种写法虽然在代码里要多几次判断,但由于执行次数有限,性能开销可以忽略。

目前这份修改仅在传参和数据集切换逻辑上做了测试,还没有做完整的训练验证,因此不能保证完全无误

至于 README:

理论上不会影响除了 train.py 以外的部分。

而原 train.py 启动命令示例没有提供参数选项示例。

如果需要示例,布尔参数的传参方式需要说明一下(例如 --start_warmup true/false),以及数据集的选择 (例如 --dataset lol_v1 ) 。

具体请作者再确认一下是否要在 README 中加入这部分说明。

commit 对照请见:
👉 ec329ef


Here’s some clarification regarding the modifications:

The changes mainly focus on argument parsing and dataset selection:

1. Argument Parsing

In the original code, bool parameters were defined with type=bool. This means any non-empty string (even 'False') would be parsed as True.

I introduced a _str2bool method so that users can correctly pass values like --start_warmup true/false. If the argument is omitted, the default value is used.

Another possible solution is to use flag-style arguments (e.g., --start_warmup to enable), but this would require renaming many parameters and might break compatibility, so I didn’t adopt it for now.

2. Avoiding Multiple argparser Conflicts

Previously, some imported files (e.g., eval.py) initialized their own argparser when imported by train.py, causing conflicts due to duplicated parameters.

Now, those files only parse arguments when executed as standalone scripts, which avoids conflicts.

3. Dataset Selection Mechanism

The original design used multiple boolean flags (--lol_v1, --lolv2_real, etc.) to specify datasets, which was not intuitive and could lead to multiple datasets being enabled simultaneously.

This has been replaced by a single --dataset xxx argument (e.g., --dataset lol_v1). The subsequent code was updated accordingly. This approach is much more user-friendly, and while it adds a few conditional checks, the overhead is negligible since this logic runs only a few times.

Currently, this PR has only been tested for argument passing and dataset switching. It hasn’t undergone full training validation, so I can’t guarantee complete correctness.
As for the README, the main updates would be:

Updating usage examples to use --dataset xxx;

Clarifying boolean argument usage (e.g., --start_warmup true/false).

Please review whether these changes should be reflected in the README.

You can check the modified parts in the commit:
👉 ec329ef

@Fediory Fediory merged commit 86d7232 into Fediory:master Oct 28, 2025
@Fediory
Copy link
Owner

Fediory commented Oct 28, 2025

非常感谢您的帮助,我已经在Readme中对您表示了感谢。

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.

2 participants