Switchout hasattr for getattr wherever possible #1605
Conversation
rhatdan
requested review from
ericcurtin,
bmahabirbu,
maxamillion,
swarajpande5,
jhjaggars,
cgruver,
slp and
engelmi
as code owners
Reviewer's GuideThis PR refactors conditional attribute existence checks across the codebase by replacing repetitive hasattr patterns with concise getattr calls and default values, simplifying argument parsing, option handling, and constructor fallbacks. Class diagram for updated argument attribute access patternsclassDiagram
class Engine {
+args
+exec_args
+add_pull_newer()
+add_network()
+add_oci_runtime()
+add_privileged_options()
+cap_add(cap)
+use_tty()
+add_subcommand_env()
+add_env_option()
+add_tty_option()
+add_detach_option()
+add_port_option()
+add_device_options()
+add_rag()
+handle_podman_specifics()
+add(newargs)
}
class Model {
+remove(args)
+get_container_name(args)
+base(args, name)
+add_oci_runtime(conman_args, args)
+add_rag(exec_args, args)
+exec_model_in_container(model_path, cmd_args, args)
+setup_mounts(model_path, args)
+build_exec_args_bench(args, model_path)
+validate_args(args)
+vllm_serve(args, exec_model_path)
+llama_serve(args, exec_model_path, chat_template_path, mmproj_path)
+handle_runtime(args, exec_args, exec_model_path)
+serve(args, quiet=False)
}
class Quadlet {
+ai_image
+name
+args
+exec_args
+_gen_image(name, image)
+_gen_name(quadlet_file)
+_gen_model_volume(quadlet_file)
+_gen_port(quadlet_file)
+_gen_rag_volume(quadlet_file)
}
class Kube {
+ai_image
+name
+args
+exec_args
}
class ModelFactory {
+pruned_model
+draft_model
+split_model
+mnt_path
+create_url()
}
class Stack {
+args
+name
+host
}
Engine --> Model
ModelFactory --> Model
Quadlet --> Model
Kube --> Model
Stack --> Engine
Stack --> Model
Stack --> Quadlet
Stack --> Kube
Stack --> ModelFactory
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 @rhatdan, 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 primarily focuses on refactoring the codebase to replace instances of hasattr() checks followed by attribute access with the more Pythonic and often cleaner getattr() function. While the main intent is to simplify code logic and improve readability, a detailed review of the changes reveals several instances where the conversion has inadvertently altered the original conditional behavior, which could lead to different program execution paths under specific conditions.
Highlights
- Code Modernization and Readability: Replaced numerous
hasattr(obj, 'attr') and obj.attrpatterns with the more concise and Pythonicgetattr(obj, 'attr', default_value)across the codebase. This improves readability and reduces boilerplate. - Consistent Attribute Access: Standardized the way optional or potentially missing attributes are handled, leveraging
getattr's ability to provide a fallback value. - Introduced Behavioral Changes: Several instances of
hasattrtogetattrconversion have subtly, or in some cases, significantly altered the original conditional logic. These changes affect how certain command-line arguments or object attributes are interpreted, potentially leading to different execution paths than before. Specific examples include logic related tonocapdrop,ARGS,subcommand,port,rag, andnameattributes in various modules.
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. ↩
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @rhatdan - I've reviewed your changes - here's some feedback:
- Consider extracting a helper (e.g.
arg_val(name, default)) to encapsulate the repeatedgetattr(self.args, ..., default)pattern and reduce duplication. - Verify that each default passed to
getattrpreserves the original truthiness semantics (e.g. distinguishing empty string vsNone) to avoid unintended behavior changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting a helper (e.g. `arg_val(name, default)`) to encapsulate the repeated `getattr(self.args, ..., default)` pattern and reduce duplication.
- Verify that each default passed to `getattr` preserves the original truthiness semantics (e.g. distinguishing empty string vs `None`) to avoid unintended behavior changes.
## Individual Comments
### Comment 1
<location> `ramalama/engine.py:80` </location>
<code_context>
"--security-opt=label=disable",
]
- if not hasattr(self.args, "nocapdrop"):
+ if getattr(self.args, "nocapdrop", False):
self.exec_args += [
"--cap-drop=all",
</code_context>
<issue_to_address>
Logic inversion may alter behavior for nocapdrop.
Previously, capabilities were dropped unless 'nocapdrop' was set. Now, they are only dropped if 'nocapdrop' is True, which reverses the intended logic and may cause capabilities to be dropped in unintended cases.
</issue_to_address>
### Comment 2
<location> `ramalama/engine.py:92` </location>
<code_context>
if not sys.stdin.isatty():
return False
- if not (hasattr(self.args, "ARGS") and self.args.ARGS):
+ if getattr(self.args, "ARGS", None):
return False
- if not (hasattr(self.args, "subcommand") and self.args.subcommand == "run"):
</code_context>
<issue_to_address>
Logic for ARGS in use_tty is now inverted.
This change may alter use_tty's behavior. Please confirm if this is intentional.
</issue_to_address>
### Comment 3
<location> `ramalama/model.py:240` </location>
<code_context>
def add_rag(self, exec_args, args):
- if not hasattr(args, "rag") or not args.rag:
+ if getattr(args, "rag", None):
return exec_args
</code_context>
<issue_to_address>
Logic for add_rag is now inverted.
Previously, the function exited if 'rag' was missing or falsy. Now, it exits if 'rag' is present and truthy, which likely prevents adding 'rag' when it should be added.
</issue_to_address>
### Comment 4
<location> `ramalama/quadlet.py:118` </location>
<code_context>
files: list[UnitFile] = []
- if not hasattr(self.args, "rag") or not self.rag:
+ if getattr(self.args, "rag", None):
return files
</code_context>
<issue_to_address>
Logic for _gen_rag_volume is now inverted.
This change causes the function to skip generating rag volumes when rag is set, which is the opposite of the previous logic.
</issue_to_address>
### Comment 5
<location> `ramalama/cli.py:273` </location>
<code_context>
def post_parse_setup(args):
"""Perform additional setup after parsing arguments."""
- if hasattr(args, "MODEL") and args.subcommand != "rm":
+ if getattr(args, "MODEL", "rm") != "rm":
resolved_model = shortnames.resolve(args.MODEL)
if resolved_model:
</code_context>
<issue_to_address>
Default value for MODEL may cause unexpected behavior.
Defaulting to 'rm' may unintentionally trigger the condition. Recommend checking if MODEL is set explicitly instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ramalama/engine.py
Outdated
| self.exec_args += ["--privileged"] | ||
| else: | ||
| self.exec_args += [ | ||
| "--security-opt=label=disable", | ||
| ] | ||
| if not hasattr(self.args, "nocapdrop"): | ||
| if getattr(self.args, "nocapdrop", False): |
There was a problem hiding this comment.
issue (bug_risk): Logic inversion may alter behavior for nocapdrop.
Previously, capabilities were dropped unless 'nocapdrop' was set. Now, they are only dropped if 'nocapdrop' is True, which reverses the intended logic and may cause capabilities to be dropped in unintended cases.
| @@ -89,9 +89,9 @@ def cap_add(self, cap): | |||
| def use_tty(self): | |||
| if not sys.stdin.isatty(): | |||
| return False | |||
| if not (hasattr(self.args, "ARGS") and self.args.ARGS): | |||
| if getattr(self.args, "ARGS", None): | |||
There was a problem hiding this comment.
question (bug_risk): Logic for ARGS in use_tty is now inverted.
This change may alter use_tty's behavior. Please confirm if this is intentional.
ramalama/model.py
Outdated
| @@ -237,7 +237,7 @@ def add_oci_runtime(self, conman_args, args): | |||
| return conman_args | |||
|
|
|||
| def add_rag(self, exec_args, args): | |||
| if not hasattr(args, "rag") or not args.rag: | |||
| if getattr(args, "rag", None): | |||
There was a problem hiding this comment.
issue (bug_risk): Logic for add_rag is now inverted.
Previously, the function exited if 'rag' was missing or falsy. Now, it exits if 'rag' is present and truthy, which likely prevents adding 'rag' when it should be added.
ramalama/quadlet.py
Outdated
| quadlet_file.add("Container", "PublishPort", f"{self.args.port}:{self.args.port}") | ||
|
|
||
| def _gen_rag_volume(self, quadlet_file: UnitFile): | ||
| files: list[UnitFile] = [] | ||
|
|
||
| if not hasattr(self.args, "rag") or not self.rag: | ||
| if getattr(self.args, "rag", None): |
There was a problem hiding this comment.
issue (bug_risk): Logic for _gen_rag_volume is now inverted.
This change causes the function to skip generating rag volumes when rag is set, which is the opposite of the previous logic.
ramalama/cli.py
Outdated
| @@ -270,7 +270,7 @@ def parse_arguments(parser): | |||
|
|
|||
| def post_parse_setup(args): | |||
| """Perform additional setup after parsing arguments.""" | |||
| if hasattr(args, "MODEL") and args.subcommand != "rm": | |||
| if getattr(args, "MODEL", "rm") != "rm": | |||
There was a problem hiding this comment.
suggestion (bug_risk): Default value for MODEL may cause unexpected behavior.
Defaulting to 'rm' may unintentionally trigger the condition. Recommend checking if MODEL is set explicitly instead.
| if getattr(args, "name", None): | ||
| return args.name | ||
|
|
||
| return genname() |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp)
| if getattr(args, "name", None): | |
| return args.name | |
| return genname() | |
| return args.name if getattr(args, "name", None) else genname() |
| @@ -291,7 +291,7 @@ def exec_model_in_container(self, model_path, cmd_args, args): | |||
| def setup_mounts(self, model_path, args): | |||
There was a problem hiding this comment.
issue (code-quality): Low code quality found in Model.setup_mounts - 17% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
There was a problem hiding this comment.
Code Review
This pull request aims to simplify the codebase by replacing hasattr with getattr, which is a great initiative for improving code clarity. While many of the changes are correct and beneficial, I've identified several instances where the new implementation incorrectly alters the original logic, introducing bugs. These include logical inversions, incorrect condition changes, and a critical issue that could lead to a crash. Please review the detailed comments to address these issues.
ramalama/cli.py
Outdated
| @@ -270,7 +270,7 @@ def parse_arguments(parser): | |||
|
|
|||
| def post_parse_setup(args): | |||
| """Perform additional setup after parsing arguments.""" | |||
| if hasattr(args, "MODEL") and args.subcommand != "rm": | |||
| if getattr(args, "MODEL", "rm") != "rm": | |||
There was a problem hiding this comment.
The logic in this condition has been changed incorrectly. The original code checked for the existence of args.MODEL AND that args.subcommand was not "rm". The new code getattr(args, "MODEL", "rm") != "rm" effectively checks if args.MODEL exists and is not "rm", or if it doesn't exist, defaults to "rm" (making the condition false).
However, the critical part args.subcommand != "rm" is completely lost. Furthermore, if args.MODEL is a list (e.g., ['mymodel'] when the subcommand is rm), getattr(args, "MODEL", "rm") would return ['mymodel'], and ['mymodel'] != "rm" would evaluate to True. This would cause the code block to execute for the rm subcommand, which was not the original intent and will lead to unexpected behavior.
To preserve the original logic while using getattr, the condition should explicitly check for the existence of args.MODEL and the subcommand value.
| if getattr(args, "MODEL", "rm") != "rm": | |
| if getattr(args, "MODEL", None) is not None and args.subcommand != "rm": |
ramalama/model.py
Outdated
| @@ -741,7 +745,7 @@ def get_available_port_if_any() -> int: | |||
|
|
|||
| def compute_serving_port(args, quiet=False) -> str: | |||
| # user probably specified a custom port, don't override the choice | |||
| if hasattr(args, "port") and args.port not in ["", str(DEFAULT_PORT)]: | |||
| if getattr(args, "port", None) not in ["", str(DEFAULT_PORT)]: | |||
There was a problem hiding this comment.
This change introduces a potential AttributeError. If args.port does not exist, getattr(args, "port", None) will return None. The condition None not in ["", str(DEFAULT_PORT)] will then evaluate to True, and the next line target_port = args.port will raise an AttributeError because args.port does not exist.
To fix this, the default value for getattr should be one of the values in the list, like "", to make the condition False when the attribute is missing.
| if getattr(args, "port", None) not in ["", str(DEFAULT_PORT)]: | |
| if getattr(args, "port", "") not in ["", str(DEFAULT_PORT)]: |
ramalama/engine.py
Outdated
| self.exec_args += ["--privileged"] | ||
| else: | ||
| self.exec_args += [ | ||
| "--security-opt=label=disable", | ||
| ] | ||
| if not hasattr(self.args, "nocapdrop"): | ||
| if getattr(self.args, "nocapdrop", False): |
There was a problem hiding this comment.
The logic for handling nocapdrop has been inverted. The original code, if not hasattr(self.args, "nocapdrop"), drops capabilities if the --nocapdrop flag is not present. The new code, if getattr(self.args, "nocapdrop", False), will not drop capabilities if the flag is absent, which is the opposite of the original behavior. This has security implications as capabilities might not be dropped when they should be.
The condition should be negated to preserve the original logic.
| if getattr(self.args, "nocapdrop", False): | |
| if not getattr(self.args, "nocapdrop", False): |
ramalama/engine.py
Outdated
| if getattr(self.args, "ARGS", None): | ||
| return False | ||
| if not (hasattr(self.args, "subcommand") and self.args.subcommand == "run"): | ||
| if getattr(self.args, "subcommand", "") == "run": | ||
| return False |
There was a problem hiding this comment.
The logic for the checks on ARGS and subcommand has been inverted. The original code used if not (condition): return False, which is equivalent to if condition: pass else: return False. The new code uses if condition: return False, which is the opposite.
This inverts the conditions under which a TTY is used. While the new logic for ARGS seems more appropriate for a TTY check (i.e., don't use TTY if ARGS are provided), this is a silent logic change that should be carefully reviewed. The inversion for subcommand is particularly suspect, as it now prevents using a TTY for the run command.
To avoid unintended side effects, it's best to stick to a direct translation of the original logic.
| if getattr(self.args, "ARGS", None): | |
| return False | |
| if not (hasattr(self.args, "subcommand") and self.args.subcommand == "run"): | |
| if getattr(self.args, "subcommand", "") == "run": | |
| return False | |
| if not getattr(self.args, "ARGS", None): | |
| return False | |
| if not (getattr(self.args, "subcommand", "") == "run"): | |
| return False |
ramalama/model.py
Outdated
| if getattr(args, "rag", None): | ||
| return exec_args |
There was a problem hiding this comment.
The logic for the rag check has been inverted. The original code was if not hasattr(args, "rag") or not args.rag:, which would skip the RAG setup if rag was not specified or was falsy. The new code if getattr(args, "rag", None): will now skip the RAG setup if rag is specified, which is the opposite of the intended behavior.
| if getattr(args, "rag", None): | |
| return exec_args | |
| if not getattr(args, "rag", None): | |
| return exec_args |
| @@ -145,7 +145,7 @@ def create_url(self) -> URL: | |||
| model = URL(self.pruned_model, urlparse(self.model).scheme) | |||
| self.set_optional_model_store(model) | |||
| model.draft_model = self.draft_model | |||
| if hasattr(self, 'split_model'): | |||
| if getattr(self, 'split_model', None): | |||
There was a problem hiding this comment.
Replacing hasattr(self, 'split_model') with getattr(self, 'split_model', None) changes the logic. hasattr only checks for the attribute's existence, whereas the new code also checks for its truthiness.
The split_model attribute can be an empty dictionary ({}), which is falsy. In this case, the original code would execute the if block because hasattr would return True. However, the new code if getattr(self, 'split_model', None): would evaluate bool({}) which is False, thus skipping the block. This could lead to model.split_model not being set correctly.
It's safer to stick with hasattr here to only check for the attribute's presence.
| if getattr(self, 'split_model', None): | |
| if hasattr(self, 'split_model'): |
ramalama/quadlet.py
Outdated
| if getattr(self.args, "rag", None): | ||
| return files |
There was a problem hiding this comment.
The logic for checking rag has been inverted. The original code if not hasattr(self.args, "rag") or not self.rag: would continue to generate the RAG volume if rag was specified. The new code if getattr(self.args, "rag", None): will return files if rag is specified, which means the RAG volume generation is skipped. This is the opposite of the intended behavior.
| if getattr(self.args, "rag", None): | |
| return files | |
| if not getattr(self.args, "rag", None): | |
| return files |
rhatdan
force-pushed
the
chat
branch
6 times, most recently
from
15cc4a1 to
b877f1a
Compare
Prune model store code Signed-off-by: Daniel J Walsh <[email protected]>
|
Shouldn't have merged, meant to approve, I'll keep an eye on the build and fix accordingly |
|
Somewhat related, I like how a bot suggested, set the default as this for me: ideally in default we should call a function that handles the, config file, env var precedence. With CLI option taking precedence over everything. |
This greatly simplifies the code logic.
Summary by Sourcery
Enhancements: