Adds a user configuration setting to disable gpu prompting #1632
Conversation
ieaves
requested review from
rhatdan,
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars,
cgruver and
engelmi
as code owners
Reviewer's GuideExtends the layered configuration system to support nested dataclass-based subconfig objects and adds a new user-level flag to disable GPU prompts, wiring it through the config initialization and prompt flow. Sequence diagram for disabling GPU prompt via user configsequenceDiagram
participant User
participant Application
participant Config
participant confirm_no_gpu
User->>Application: Start operation requiring GPU
Application->>Config: Access CONFIG.user.no_missing_gpu_prompt
Config-->>Application: Return True (if set)
Application->>confirm_no_gpu: Call function
confirm_no_gpu->>Config: Check CONFIG.user.no_missing_gpu_prompt
alt no_missing_gpu_prompt is True
confirm_no_gpu-->>Application: Return True (skip prompt)
else no_missing_gpu_prompt is False
confirm_no_gpu->>User: Prompt for confirmation
User-->>confirm_no_gpu: Respond
confirm_no_gpu-->>Application: Return result
end
Class diagram for nested configuration and UserConfig additionclassDiagram
class LayeredMixin {
+__init__(*layers)
+is_set(name)
}
class BaseConfig {
+container: bool
+engine: str
+store: str
+model: str
+transport: str
+ocr: bool
+default_image: str
+user: UserConfig
+__post_init__()
}
class UserConfig {
+no_missing_gpu_prompt: bool
}
BaseConfig --> UserConfig : has-a
LayeredMixin <|-- BaseConfig
Class diagram for build_subconfigs utility functionclassDiagram
class build_subconfigs {
+build_subconfigs(values: dict, obj) dict
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ieaves - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `ramalama/layered_config.py:20` </location>
<code_context>
+ """Facilitates nesting configs by instantiating the child typed object from a dict"""
+ dtypes: dict[str, Type] = obj.__annotations__
+ for k, v in values.items():
+ if isinstance(v, dict) and (subconfig_type := dtypes.get(k)) and is_dataclass(subconfig_type):
+ values[k] = subconfig_type(**build_subconfigs(v, dtypes[k]))
+ return values
+
</code_context>
<issue_to_address>
Modifying the input dictionary in-place may have unintended side effects.
Mutating 'values' directly can cause issues if it's used elsewhere. Recommend returning a new dictionary to prevent side effects.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if isinstance(v, dict) and (subconfig_type := dtypes.get(k)) and is_dataclass(subconfig_type): | ||
| values[k] = subconfig_type(**build_subconfigs(v, dtypes[k])) |
There was a problem hiding this comment.
issue (bug_risk): Modifying the input dictionary in-place may have unintended side effects.
Mutating 'values' directly can cause issues if it's used elsewhere. Recommend returning a new dictionary to prevent side effects.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ieaves, 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 enhances the configuration system by allowing nested configurations and introduces a user-configurable setting to disable the GPU prompt. This change aims to provide more flexibility in configuring the application and addresses a specific user request.
Highlights
- Configuration Nesting: Introduces the ability to nest configuration settings as structured objects using dataclasses.
- Disable GPU Prompt: Adds a user configuration option to disable the 'no GPU found' prompt, addressing issue #1618.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
This morning I thought this was going to be an easy fix but once I started running unit tests I belatedly realized the issue was more complicated than it first appeared because of a circular import issue and the dependency tree between common and config. In order to break the cyclical dependency I needed to pull apple_vm out of the get_default_engine logic being used in the config which is in any case a good idea as otherwise we end up calling it multiple times (which itself involves a subprocess call to the podman cli). The equivalent logic for OSX has been moved to the Next up, I needed a place to store the new prompt disabling configuration setting. I went back and forth on either setting up a new local cache for user preferences using something like shelve but anticipated some hesitation around splitting the config and introducing a second file which has to be managed. I landed, instead, on leaving it in the default config but under a nested It was at this point that I realized modifying the config file in a testing environment was slightly complicated by the multiple config file locations we traverse and that, for the purposes of the initial issue, a better resolution would be to allow these settings to be defined through environment variables. In order to do that I implemented a pattern for the arbitrary resolution of config values in environment variables where the config value [ramalama.subkey]
key = valuecorresponds to the environment variable Finally, I added runtime type validation logic for these environmental variables. With that machinery in place I added changes in conftest to preload specific environment variables for testing purposes and then restore the users environment once testing completed. |
ieaves
force-pushed
the
feat/user-prompt-configs
branch
2 times, most recently
from
ed2d303 to
45fce73
Compare
Signed-off-by: Ian Eaves <[email protected]>
ieaves
force-pushed
the
feat/user-prompt-configs
branch
from
45fce73 to
27fa390
Compare
| def coerce_to_bool(value: Any) -> bool: | ||
| if isinstance(value, bool): | ||
| return value | ||
| elif isinstance(value, str): |
There was a problem hiding this comment.
Nit: No need for elif, if works fine.
| val = value.strip().lower() | ||
| if val in {"true", "1", "yes", "y"}: | ||
| return True | ||
| elif val in {"false", "0", "no", "n"}: |
There was a problem hiding this comment.
Nit: No need for elif, if works fine.
|
LGTM |
This PR provides two pieces of functionality
If we like this path, I will modify the tests to disable the gpu prompting with a fixture.
Summary by Sourcery
Enable structured nested configurations and add a user-level option to bypass GPU missing prompts
New Features:
user.no_missing_gpu_promptsetting to disable GPU missing promptsEnhancements:
build_subconfigsinLayeredMixinto instantiate nested config dataclassesconfirm_no_gputo respect the newno_missing_gpu_promptuser setting