Include custom Python CLIs in generated SDKs - #50
Conversation
|
👋 Hello @JaviChulvi, thank you for submitting a
For more guidance, please refer to our Contributing Guide. Don't hesitate to leave a comment if you have any questions. Thank you for contributing to Ultralytics! 🚀 |
UltralyticsAssistant
left a comment
There was a problem hiding this comment.
🔍 PR Review
Made with ❤️ by Ultralytics Actions
Reviewed the configuration validation, Python generation, multipart metadata assembly, package-script registration, cleanup behavior, and focused test. The opt-in path is otherwise scoped and preserves default generation, but source-side __main__ launchers can cause failure or duplicate command execution when the generated module is run directly.
💬 Posted 1 inline comment
- 💡 MEDIUM
lib/generators/python.ts:885Appending the generated launcher after the consumer source makes a normal executablecli.pywith its ownif __name__ == "__main__": main()block unsafe: direct execution runsmain()beforeMULTIPART_FILESis assigned, which can raiseNameError, or runs the command twice. The documented contract only requires a zero-argumentmain()and does not forbid a source-side launcher. Reject or strip an existing launcher, or explicitly enforce a module-only source contract before generating th…
UltralyticsAssistant
left a comment
There was a problem hiding this comment.
🔍 PR Review 2
Made with ❤️ by Ultralytics Actions
Since review 1, the source-side launcher issue is addressed by rejecting consumer modules that reference __main__, with focused coverage for both successful generation and rejection. I also reviewed configuration validation, multipart metadata, package-script registration, cleanup behavior, and header insertion. No remaining behavioral defects found. LGTM.
UltralyticsAssistant
left a comment
There was a problem hiding this comment.
🔍 PR Review 3
Made with ❤️ by Ultralytics Actions
Since review 2, the prior premature-execution issue is addressed by keeping the consumer launcher unchanged and moving multipart metadata to a separate module. Two risks remain: the focused test only validates direct-file execution with an import that fails for packaged console/module entrypoints, and the README still describes the removed generator-owned launcher contract.
💬 Posted 2 inline comments
- 💡 MEDIUM
lib/generators/python.test.ts:106This fixture imports_cli_metadataas a top-level module and is only executed by filename on line 117, where the package directory happens to be onsys.path. The registered console script andpython -m <package>.cliimport<package>.cli;_cli_metadata.pyis a sibling package module, so this source raisesModuleNotFoundErrorthere. Use a package-relative import and exercise the registered/module entrypoints so the core CLI path is actually validated. - 📝 LOW
README.md:59This documents the behavior removed by the current implementation: the generator no longer appendsMULTIPART_FILESor a__main__block, and it no longer rejects__main__; the consumer source now owns its launcher and must import the generated metadata module. As written, consumers following this new configuration will receive an obsolete contract and can generate a non-working CLI. Update this paragraph before release.
UltralyticsAssistant
left a comment
There was a problem hiding this comment.
🔍 PR Review 4
Made with ❤️ by Ultralytics Actions
Since review 3, both reported issues are addressed: the fixture now uses a package-relative metadata import and exercises both module and registered console entrypoints, while the README documents the consumer-owned launcher and generated metadata contract. No remaining or newly introduced behavioral defects found. LGTM.
Why
SDK repositories need to ship their maintained CLI through normal package regeneration. Optional
python.cliconfiguration lets the SDK install a command such asulwhile keeping product-specific behavior in the SDK repository.Design
The generator copies the CLI source unchanged, registers its console command, and writes a small module identifying multipart file fields that SDK annotations cannot describe. The consumer owns
main()and its module launcher; SDK signatures and docstrings provide the remaining argument information.Keeping metadata in a separate module lets ordinary imports handle initialization and preserves the consumer's docstrings and future imports. Existing SDK generation stays unchanged when CLI support is not configured.
Validation: 20 generator tests, typecheck, lint, and unused-code checks pass. Package tests run both the console command and module launcher, and verify cleanup when CLI support is disabled.
Merge before sdk#59, whose CI regenerates from generator
main.