Skip to content

Conversation

@zhulinJulia24
Copy link
Contributor

Initializing the testing framework allows for quick addition of sft、rl and pretrain related test cases through configuration.
On the quality side, focus on the code development of the validation and regression points themselves(train、infer、eval and so on).

Comment on lines +5 to +9
repo_org:
required: false
description: 'Tested repository organization name. Default is InternLM'
type: string
default: 'InternLM/xtuner'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this arguments is required?


def execute_task(self, task_config: Dict[str, Any]) -> Dict[str, Any]:
resource = task_config.get("resource", {})
command = task_config.get("command", "")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest directly returning False here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, it make sense

self.cluster = cluster
self.params_cls = params_cls

def execute_task(self, task_config: Dict[str, Any]) -> Dict[str, Any]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest declaring a TypedDict or Pydantic BaseModel here for easier checking of config correctness.

self.cluster = cluster
self.params_cls = params_cls

def execute_task(self, task_config: Dict[str, Any]) -> Dict[str, Any]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miss matched return statement.

-
type: sft
parameters:
config: /mnt/shared-storage-user/llmrazor-share/qa-llm-cicd/xtuner-fork/autotest/config/qwen3.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest using relative path

config = get_config()
case_list = config["case"]

if type == "all":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is a built-in keyword in python, suggest renaming it

- HF_DATASETS_OFFLINE=1
- HF_HUB_OFFLINE=1

case:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hierarchy here seems unusual. From the original semantics, the xtuner repository should have the following test levels:

  • Tasking: sft & pretrain & rl
  • Training config: config path
  • Resource: test resource

Xtuner's configuration files can determine the first two, while the last one is determined by clusterx's configuration. What do you think about adjusting the hierarchy like this:

case:
  - task: <rl | sft | pretrain>
    config: <config path>
    assert_info: <...>
    resources: [<resource_name, including env>]

Then resources should contain test resources like gpu, npu, etc., with different names. In each case item, you can select which platforms and resource specifications to test on.

return env_config


def get_case_list(type: str = "all"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to consider how to platform specific test case

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