Skip to content

Conversation

@MNGanesan
Copy link
Contributor

Presently --help for vitis displays the target and option string,
it has no description. Eg: target vitis-ai dpu<class 'str'>

This can be made more meaningful by fetching the description from the config node of the target. Eg: Vitis AI DPU identifier

Signed-off-by: MNGanesan mnganesan@yahoo.co.uk

@tvm-bot
Copy link
Collaborator

tvm-bot commented Jan 17, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

@MNGanesan
Copy link
Contributor Author

MNGanesan commented Jan 17, 2023

Present --help string has no meaningful description, it simply prints the target, option name and python type


target vitis-ai:
  --target-vitis-ai-dpu TARGET_VITIS_AI_DPU
                        **target vitis-ai dpu<class 'str'>**
  --target-vitis-ai-build_dir TARGET_VITIS_AI_BUILD_DIR
                        **target vitis-ai build_dir<class 'str'>**
  --target-vitis-ai-work_dir TARGET_VITIS_AI_WORK_DIR
                        **target vitis-ai work_dir<class 'str'>**
  --target-vitis-ai-export_runtime_module TARGET_VITIS_AI_EXPORT_RUNTIME_MODULE
                        **target vitis-ai export_runtime_module<class 'str'>**
  --target-vitis-ai-load_runtime_module TARGET_VITIS_AI_LOAD_RUNTIME_MODULE
                        **target vitis-ai load_runtime_module<class 'str'>**

This can be enhanced by accessing the config node of codegen target, which has meaningful descritption.
It is retrieved in the front end using the "field.description" of config

struct VitisAICompilerConfigNode : public tvm::AttrsNode<VitisAICompilerConfigNode> {
  String dpu;
  String build_dir;
  String work_dir;
  String export_runtime_module;
  String load_runtime_module;
  TVM_DECLARE_ATTRS(VitisAICompilerConfigNode, "ext.attrs.VitisAICompilerConfigNode") {
    TVM_ATTR_FIELD(dpu).describe("Vitis AI DPU identifier").set_default("");
    TVM_ATTR_FIELD(build_dir)
        .describe("Build directory to be used (optional, debug)")
        .set_default("");
    TVM_ATTR_FIELD(work_dir)
        .describe("Work directory to be used (optional, debug)")
        .set_default("");
    TVM_ATTR_FIELD(export_runtime_module)
        .describe("Export the Vitis AI runtime module to this file")
        .set_default("");
    TVM_ATTR_FIELD(load_runtime_module)

Enhanced --help string after the fix

target vitis-ai:

  --target-vitis-ai-dpu TARGET_VITIS_AI_DPU
                        **Vitis AI DPU identifier**
  --target-vitis-ai-build_dir TARGET_VITIS_AI_BUILD_DIR
                        **Build directory to be used (optional, debug)**
  --target-vitis-ai-work_dir TARGET_VITIS_AI_WORK_DIR
                        **Work directory to be used (optional, debug)**
  --target-vitis-ai-export_runtime_module TARGET_VITIS_AI_EXPORT_RUNTIME_MODULE
                        **Export the Vitis AI runtime module to this file**
  --target-vitis-ai-load_runtime_module TARGET_VITIS_AI_LOAD_RUNTIME_MODULE
                        **Load the Vitis AI runtime module to this file**

@MNGanesan MNGanesan changed the title Enhance the --help message of composite target's option description Enhance the --help message of composite target Jan 17, 2023
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Thanks @MNGanesan this looks like a sensible change to me. Would it be possible to add a test to check this case perhaps in tests/python/driver/tvmc/test_target_options.py?

Also cc @Mousius

@lhutton1 lhutton1 changed the title Enhance the --help message of composite target [TVMC] Enhance the --help message of composite target Jan 17, 2023
@Mousius Mousius changed the title [TVMC] Enhance the --help message of composite target [TVMC] Enhance the --help message of codegen target Jan 17, 2023
Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

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

This looks like an awesome change, the original author made an odd choice here 🤔

Please add a test 😸

@MNGanesan
Copy link
Contributor Author

Thanks @lhutton1 @Mousius , I'll working on the test and add it shortly

@leandron
Copy link
Contributor

@tvm-bot rerun

MNGanesan added a commit to MNGanesan/tvm that referenced this pull request Jan 24, 2023
via the pull request apache#13798

Here, adding test cases to verify the fix

Signed-off-by: MNGanesan <mnganesan@yahoo.co.uk>
@MNGanesan
Copy link
Contributor Author

Added the test via commit

commit f6bfd9a (HEAD -> main, origin/main, origin/HEAD)
Author: MNGanesan mnganesan@yahoo.co.uk
Date: Tue Jan 24 13:58:00 2023 +0000

@MNGanesan
Copy link
Contributor Author

MNGanesan commented Jan 24, 2023 via email

@MNGanesan
Copy link
Contributor Author

MNGanesan commented Jan 24, 2023 via email

Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test @MNGanesan, it looks good to me :)

I took a quick look at the CI failures and they seem unrelated to your change. I wonder if your branch may need to be rebased onto the latest main?

@MNGanesan
Copy link
Contributor Author

Dropping this PR, moved by fix and test to new PR #13842 under my branch enhance_help_msg

@MNGanesan
Copy link
Contributor Author

MNGanesan commented Jan 25, 2023 via email

@MNGanesan
Copy link
Contributor Author

MNGanesan commented Jan 25, 2023 via email

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.

5 participants