-
Notifications
You must be signed in to change notification settings - Fork 671
Fix duplicate engine config update in TurboMindModelwithChatTemplate #2276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix duplicate engine config update in TurboMindModelwithChatTemplate #2276
Conversation
The duplicate update will cause AttributeError with plain dicts as it has no attribute 'to_dict'.
else: | ||
raise ValueError(f'expected Dict or ConfigDict engine_config but got {type(engine_config)}') | ||
|
||
_engine_config.update(engine_config.to_dict()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
The motivation for calling engine_config.to_dict()
is that we discovered it's an instance of mmengine.ConfigDict
, which is not compatible with the lmdeploy turbomind engine. For reference, see these related PRs:
- Override HF config.json via CLI InternLM/lmdeploy#3722
- minor fix about the log level and logs InternLM/lmdeploy#3758
To make this more robust, I'd suggest the following approach:
try:
if engine_config is not None:
_engine_config.update(engine_config.to_dict())
except AttributeError:
logger.warning("engine_config has no to_dict method")
except TypeError:
logger.warning("engine_config.to_dict() did not return a dictionary")
except Exception as e:
logger.error(f"Unexpected error: {e}")
Additionally, could you please share the steps to reproduce the issue you're addressing? This will help us better understand the context and ensure comprehensive testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your feedback and for providing the context behind the original implementation.
Here are the detailed steps on how I encountered and reproduced the issue:
I'm new to OpenCompass and was evaluating the performance of the Qwen2.5 model on some datasets. And I use the LMDeploy inference engine for acceleration. I followed the tutorial to create a configuration file (eval_qwen.py
), which I've simplified below.
from mmengine.config import read_base
with read_base():
from opencompass.configs.datasets.cmmlu.cmmlu_gen import cmmlu_datasets
datasets = cmmlu_datasets[:3]
from opencompass.models import TurboMindModelwithChatTemplate
# Unified parameter configuration for ease of use and modification
model_path = ".../LLM/Qwen/Qwen2.5-7B-Instruct"
abbr = "qwen2_5_7B"
backend = "turbomind"
max_out_len = 4096
batch_size = 8
engine_config={'tp': 1},
gen_config={'do_sample': False},
models = [
dict(
type=TurboMindModelwithChatTemplate,
abbr=abbr,
path=model_path,
####################
# Case 1: Works well
# Directly specifying the value as per the tutorial
# engine_config=dict(tp=1),
# gen_config=dict(do_sample=False),
# Case 2: Raises error (ValueError: expected Dict or ConfigDict engine_config but got <class 'tuple'>)
# Using pre-defined variables
# engine_config=engine_config,
# gen_config=gen_config,
# Case 3: Raises error (AttributeError: 'dict' object has no attribute 'to_dict')
# Not specifying these parameters at all (relying on defaults)
####################
backend=backend,
max_out_len=max_out_len,
batch_size=batch_size,
run_cfg=dict(num_gpus=1),
),
]
The used execution command is:
opencompass eval_qwen.py --debug -a lmdeploy
As shown in eval_qwen.py
, I tried three ways to specify the engine_config and gen_config parameters:
- Case1: Following the tutorial's approach by directly specifying their values. This case worked normally.
- Case2: For the convenience of parameter settings, I intended to predefine variable values and then pass these variables instead of specific values. However, this method caused errors. From the log file in debug mode, I saw:
# 1842737_debug.log
...
Traceback (most recent call last):
File ".../lib/python3.10/site-packages/opencompass/tasks/openicl_infer.py", line 161, in <module>
inferencer.run()
File ".../lib/python3.10/site-packages/opencompass/tasks/openicl_infer.py", line 73, in run
self.model = build_model_from_cfg(model_cfg)
File ".../lib/python3.10/site-packages/opencompass/utils/build.py", line 24, in build_model_from_cfg
return MODELS.build(model_cfg)
File ".../lib/python3.10/site-packages/mmengine/registry/registry.py", line 570, in build
return self.build_func(cfg, *args, **kwargs, registry=self)
File ".../lib/python3.10/site-packages/mmengine/registry/build_functions.py", line 121, in build_from_cfg
obj = obj_cls(**args) # type: ignore
File ".../lib/python3.10/site-packages/opencompass/models/turbomind_with_tf_above_v4_33.py", line 68, in __init__
raise ValueError(f'expected Dict or ConfigDict engine_config but got {type(engine_config)}')
ValueError: expected Dict or ConfigDict engine_config but got <class 'tuple'>
...
The type of engine_config became tuple. Later I tried to print its content, and the output was: ({'tp': 1},)
. This means the originally defined dict variable (i.e., {'tp': 1}
) was converted into a tuple form. OpenCompass might have some preprocessing mechanism that I am not yet aware of.
- Case3: After encountering the problem in Case2, I thought about not specifying the values for engine_config and gen_config and letting them keep their default values. So I removed the explicit parameter specification. This is when I finally encountered this error:
# 1856164_debug.log
...
Traceback (most recent call last):
File ".../lib/python3.10/site-packages/opencompass/tasks/openicl_infer.py", line 161, in <module>
inferencer.run()
File ".../lib/python3.10/site-packages/opencompass/tasks/openicl_infer.py", line 73, in run
self.model = build_model_from_cfg(model_cfg)
File ".../lib/python3.10/site-packages/opencompass/utils/build.py", line 24, in build_model_from_cfg
return MODELS.build(model_cfg)
File ".../lib/python3.10/site-packages/mmengine/registry/registry.py", line 570, in build
return self.build_func(cfg, *args, **kwargs, registry=self)
File ".../lib/python3.10/site-packages/mmengine/registry/build_functions.py", line 121, in build_from_cfg
obj = obj_cls(**args) # type: ignore
File ".../lib/python3.10/site-packages/opencompass/models/turbomind_with_tf_above_v4_33.py", line 70, in __init__
_engine_config.update(engine_config.to_dict())
AttributeError: 'dict' object has no attribute 'to_dict'
...
According to the definition in the init method (engine_config: Dict|ConfigDict = {}
), if engine_config
is not explicitly specified, it is created by default as {}
. Do we have to explicitly specify the values of these parameters?
I have just started using OpenCompass and am not very familiar with its specific design and implementation details. I hope this information can be helpful and wish OpenCompass continued improvement and success . Thank you!
Hi, @JYLiAo32 I can reproduced the issue using your example.
As for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The
_engine_config.update(engine_config.to_dict())
line is executed after the conditional block. This causes anAttributeError
whenengine_config
is a plain Pythondict
(asdict
objects have no.to_dict()
method), resulting in an AttributeError.When
engine_config
is aConfigDict
, the operation is also redundant.