chore: add flamepy.Runner.#298
Conversation
Signed-off-by: Klaus Ma <klausm@nvidia.com>
Summary of ChangesHello @k82cn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces the flamepy.Runner API, a significant feature aimed at simplifying the deployment of Python applications on Flame. The changes encompass new SDK components for automatic packaging, uploading, and service registration, along with corresponding configurations, comprehensive documentation, and tests. The overall implementation is robust and well-structured. My review includes feedback on a documentation inconsistency, some minor improvements for documentation clarity, and suggestions to enhance code style and robustness in the test and runtime components.
|
|
||
| 1. **Storage**: Only `file://` URLs supported (S3, HTTP planned for future) | ||
| 2. **Pickling**: Execution objects must be picklable | ||
| 3. **Package Reuse**: No caching yet (always uploads new package) |
There was a problem hiding this comment.
The documentation in the "Limitations" section states that there is "No caching yet (always uploads new package)". This contradicts the implementation in sdk/python/src/flamepy/runner.py, where the _upload_package method checks if a package with the same name already exists and skips the upload. Please update the documentation to accurately reflect this behavior.
| ## Summary | ||
|
|
||
| This change provides: | ||
| - ✅ Predictable working directories (`/opt/{name}`) | ||
| - ✅ Organized file system layout | ||
| - ✅ Better debugging and troubleshooting | ||
| - ✅ Application isolation | ||
| - ✅ Fully backward compatible | ||
|
|
||
| No code changes required for existing applications! |
|
|
||
| def test_flmrun_class_method(): | ||
| """Test Case 2: Run methods on a class instance.""" | ||
| from flamepy.cache import get_object |
There was a problem hiding this comment.
The import from flamepy.cache import get_object is repeated within several test functions. According to Python's PEP 8 style guide, imports should be placed at the top of the file. Consolidating this import at the top level will improve code readability and maintainability.
References
- Imports should generally be placed at the top of the file, after any module docstrings and before any module-level global variables and constants. This practice improves readability and avoids repeated imports within functions. (link)
|
|
||
| # Get the working directory (default to /tmp if not set) | ||
| working_dir = os.getcwd() | ||
| extract_dir = os.path.join(working_dir, f"extracted_{os.path.basename(package_path).split('.')[0]}") |
There was a problem hiding this comment.
The logic to generate the extraction directory name using os.path.basename(package_path).split('.')[0] is not fully robust. It may produce unexpected results for filenames containing multiple dots, such as version numbers (e.g., my-app-1.2.3.tar.gz would result in my-app-1). Consider a more reliable method for stripping archive extensions to handle such cases correctly.
No description provided.