fix gpu thor #719
Merged
Merged
Conversation
…ion-issue-xtlmzz Fix NVML GPU warning on Thor
Contributor
Reviewer's GuideThis PR refactors the installation of environment variables and systemd services to locate templates dynamically (from package sources, distribution metadata, or shared dirs), enhances GPU detection logic to normalize and filter device names, adds a top-level "--install-service" CLI command for full setup, and updates service-related error messages to point to the new command. Sequence diagram for the new --install-service CLI workflowsequenceDiagram
actor User
participant CLI as "jtop CLI"
participant OS as "Operating System"
participant Variables as "install_variables()"
participant ServicePerm as "set_service_permission()"
participant Service as "install_service()"
User->>CLI: Run "jtop --install-service"
CLI->>OS: Check superuser privileges
alt Not superuser
CLI->>User: Print "Install requires superuser privileges"
CLI->>OS: Exit
else Is superuser
CLI->>Variables: install_variables(package_root, copy)
CLI->>ServicePerm: set_service_permission()
CLI->>Service: install_service(package_root, copy)
CLI->>User: Print "jtop.service installed and enabled"
CLI->>OS: Exit
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- You’ve introduced nearly identical template lookup logic in jetson_variables.py and service.py; consider extracting it into a shared helper to avoid duplication and ease maintenance.
- The template path resolution scans many directories on every install; consider caching resolved paths or limiting the search scope for better performance.
- Path handling is mixed between os.path and pathlib—migrating fully to pathlib could simplify code and improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You’ve introduced nearly identical template lookup logic in jetson_variables.py and service.py; consider extracting it into a shared helper to avoid duplication and ease maintenance.
- The template path resolution scans many directories on every install; consider caching resolved paths or limiting the search scope for better performance.
- Path handling is mixed between os.path and pathlib—migrating fully to pathlib could simplify code and improve readability.
## Individual Comments
### Comment 1
<location> `jtop/__main__.py:102-106` </location>
<code_context>
+ print("Install requires superuser privileges. Please run:\nsudo jtop --install-service")
+ sys.exit(1)
+ logging.basicConfig(level=logging.INFO, format='[%(levelname)s] %(name)s - %(message)s')
+ package_root = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
+ is_developer = os.path.isdir(os.path.join(package_root, 'tests'))
+ copy = not is_developer
</code_context>
<issue_to_address>
**suggestion:** The logic for determining package_root may not be robust in all installation scenarios.
This approach may fail if the directory structure differs from expectations. Using importlib.resources or pkg_resources can provide a more reliable way to locate package resources.
```suggestion
import importlib.resources
try:
# Try to get the package root using importlib.resources
package_root = importlib.resources.files("jtop").parent
package_root = str(package_root)
except Exception:
# Fallback to previous method if importlib.resources fails
package_root = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
is_developer = os.path.isdir(os.path.join(package_root, 'tests'))
copy = not is_developer
if not is_developer:
package_root = None
```
</issue_to_address>
### Comment 2
<location> `jtop/core/gpu.py:166-168` </location>
<code_context>
- # Extract real path GPU device
- path = os.path.realpath(os.path.join(item_path, "device"))
- frq_path = os.path.realpath(item_path)
- igpu[name] = {'type': 'integrated', 'path': path, 'frq_path': frq_path}
- logger.info(f"GPU \"{name}\" status in {path}")
- logger.info(f"GPU \"{name}\" frq in {frq_path}")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using the original device name as the dictionary key may cause inconsistencies.
Using unnormalized device names as keys can result in inconsistent dictionary access due to differences in case or whitespace. Use normalized_name to ensure reliable lookups.
```suggestion
normalized_name = name.strip().lower().replace(" ", "_")
igpu[normalized_name] = {'type': 'integrated', 'path': path, 'frq_path': frq_path}
logger.info(f"GPU \"{normalized_name}\" status in {path}")
logger.info(f"GPU \"{normalized_name}\" frq in {frq_path}")
```
</issue_to_address>
### Comment 3
<location> `jtop/__main__.py:98` </location>
<code_context>
def main():
parser = argparse.ArgumentParser(
description='jtop is system monitoring utility and runs on terminal',
formatter_class=argparse.ArgumentDefaultsHelpFormatter)
parser.add_argument('--force', dest='force', help=argparse.SUPPRESS, action="store_true", default=False)
parser.add_argument('--health', dest="health", help='Status jtop and fix', action="store_true", default=False)
parser.add_argument('--install-service', dest="install_service", help='Install and enable the jtop systemd service',
action="store_true", default=False)
parser.add_argument('--error-log', dest="log", help='Generate a log for GitHub', action="store_true", default=False)
parser.add_argument('--no-warnings', dest="no_warnings", help='Do not show warnings', action="store_true", default=False)
parser.add_argument('--restore', dest="restore", help='Reset Jetson configuration', action="store_true", default=False)
parser.add_argument('--loop', dest="loop", help='Automatically switch page every {sec}s'.format(sec=LOOP_SECONDS), action="store_true", default=False)
parser.add_argument('--color-filter', dest="color_filter",
help='Change jtop base colors, you can use also JTOP_COLOR_FILTER=True', action="store_true", default=False)
parser.add_argument('-r', '--refresh', dest="refresh", help='refresh interval', type=int, default='1000')
parser.add_argument('-p', '--page', dest="page", help='Open fix page', type=int, default=1)
parser.add_argument('-v', '--version', action='version', version='%(prog)s {version}'.format(version=get_var(VERSION_RE)))
# Parse arguments
args = parser.parse_args()
# Install jtop service if requested
if args.install_service:
if os.geteuid() != 0:
print("Install requires superuser privileges. Please run:\nsudo jtop --install-service")
sys.exit(1)
logging.basicConfig(level=logging.INFO, format='[%(levelname)s] %(name)s - %(message)s')
package_root = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
is_developer = os.path.isdir(os.path.join(package_root, 'tests'))
copy = not is_developer
if not is_developer:
package_root = None
try:
from .core.jetson_variables import install_variables
from .service import set_service_permission, install_service
install_variables(package_root, copy=copy)
set_service_permission()
install_service(package_root, copy=copy)
except JtopException as e:
print(e)
sys.exit(1)
print("jtop.service installed and enabled")
sys.exit(0)
# Initialize signals
# signal.signal(signal.SIGINT, exit_signal) # Do not needed equivalent to exception KeyboardInterrupt
signal.signal(signal.SIGTERM, exit_signal)
# Run jtop service
if os.getenv('JTOP_SERVICE', False):
# Initialize logging level
logging.basicConfig(level=logging.INFO, filemode='w', format='[%(levelname)s] %(name)s - %(message)s')
# Run service
try:
# Initialize stats server
server = JtopServer(force=args.force)
server.loop_for_ever()
except JtopException as e:
print(e)
# Close service
sys.exit(0)
# Initialize logging level
logging.basicConfig()
# Convert refresh to second
interval = float(args.refresh / 1000.0)
# Restore option
if args.restore:
try:
with jtop(interval=interval) as jetson:
# Write warnings
if 'L4T' in jetson.board['hardware']:
warning_messages(jetson, args.no_warnings)
# Restore configuration
if jetson.ok():
for status, name in jetson.restore():
status = bcolors.ok() if status else bcolors.fail()
print(" [{status}] {name}".format(name=name, status=status))
except JtopException as e:
print(e)
# Close service
sys.exit(0)
# Run health jtop
if args.health:
jtop_config()
# Generate a log for GitHub
if args.log:
body = get_hardware_log()
body += "\n\nLog from jtop {version}\n".format(version=get_var(VERSION_RE))
with open('{cwd}/{name}'.format(cwd=os.getcwd(), name=JTOP_LOG_NAME), 'w') as writer:
writer.write(body)
print("LOG '{name}' generated in {path}".format(name=JTOP_LOG_NAME, path=os.getcwd()))
sys.exit(0)
# jtop client start
try:
# https://stackoverflow.com/questions/56373360/n-curses-within-python-how-to-catch-and-print-non-ascii-character
# Commented for issues #466 #393
# locale.setlocale(locale.LC_ALL, '')
# Open jtop client
with jtop(interval=interval) as jetson:
# Call the curses wrapper
color_filter = bool(os.getenv('JTOP_COLOR_FILTER', args.color_filter))
# Build list pages available
pages = [ALL]
if jetson.gpu:
pages += [GPU]
pages += [CPU, MEM]
if jetson.engine:
pages += [ENGINE]
if jetson.fan or jetson.jetson_clocks is not None or jetson.nvpmodel is not None:
pages += [CTRL]
pages += [INFO]
curses.wrapper(JTOPGUI, jetson, pages, init_page=args.page,
loop=args.loop, seconds=LOOP_SECONDS, color_filter=color_filter)
# Write warnings
if 'L4T' in jetson.board['hardware']:
warning_messages(jetson, args.no_warnings)
except (KeyboardInterrupt, SystemExit):
pass
except JtopException as e:
print(e)
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into function [×2] ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>
### Comment 4
<location> `jtop/core/jetson_variables.py:406` </location>
<code_context>
def _variables_template_path(package_root, name):
candidates = []
if package_root:
base = Path(package_root)
candidates.append(base / 'scripts' / name)
candidates.append(base / name)
distribution_path = _resolve_distribution_path(
Path('scripts') / name,
Path('share/jetson_stats') / name,
Path(name),
)
if distribution_path:
candidates.append(distribution_path)
shared_roots = {
Path(prefix)
for prefix in (
sys.prefix,
getattr(sys, 'base_prefix', None),
sys.exec_prefix,
getattr(sys, 'base_exec_prefix', None),
os.environ.get('VIRTUAL_ENV'),
)
if prefix
}
try:
import sysconfig
except ImportError:
sysconfig = None
if sysconfig:
data_path = sysconfig.get_paths().get('data')
if data_path:
shared_roots.add(Path(data_path))
try:
import site
except ImportError:
site = None
if site:
user_base = site.getuserbase()
if user_base:
shared_roots.add(Path(user_base))
for root in shared_roots:
candidates.append(root / 'share' / 'jetson_stats' / name)
for candidate in candidates:
if candidate.exists():
return candidate
raise JtopException('Unable to locate {name} template file'.format(name=name))
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional [×3] ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Replace a for append loop with list extend ([`for-append-to-extend`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/for-append-to-extend/))
- Merge consecutive list appends into a single extend ([`merge-list-appends-into-extend`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-list-appends-into-extend/))
</issue_to_address>
### Comment 5
<location> `jtop/service.py:128` </location>
<code_context>
def _service_template_path(package_root, name):
candidates = []
if package_root:
base = Path(package_root)
candidates.append(base / 'services' / name)
candidates.append(base / name)
distribution_path = _resolve_distribution_path(
Path('services') / name,
Path('share/jetson_stats') / name,
Path(name),
)
if distribution_path:
candidates.append(distribution_path)
shared_roots = {
Path(prefix)
for prefix in (
sys.prefix,
getattr(sys, 'base_prefix', None),
sys.exec_prefix,
getattr(sys, 'base_exec_prefix', None),
os.environ.get('VIRTUAL_ENV'),
)
if prefix
}
try:
import sysconfig
except ImportError:
sysconfig = None
if sysconfig:
data_path = sysconfig.get_paths().get('data')
if data_path:
shared_roots.add(Path(data_path))
try:
import site
except ImportError:
site = None
if site:
user_base = site.getuserbase()
if user_base:
shared_roots.add(Path(user_base))
for root in shared_roots:
candidates.append(root / 'share' / 'jetson_stats' / name)
for candidate in candidates:
if candidate.exists():
return candidate
raise JtopException('Unable to locate {name} template file'.format(name=name))
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional [×3] ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Replace a for append loop with list extend ([`for-append-to-extend`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/for-append-to-extend/))
- Merge consecutive list appends into a single extend ([`merge-list-appends-into-extend`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-list-appends-into-extend/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+102
to
+106
| package_root = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) | ||
| is_developer = os.path.isdir(os.path.join(package_root, 'tests')) | ||
| copy = not is_developer | ||
| if not is_developer: | ||
| package_root = None |
Contributor
There was a problem hiding this comment.
suggestion: The logic for determining package_root may not be robust in all installation scenarios.
This approach may fail if the directory structure differs from expectations. Using importlib.resources or pkg_resources can provide a more reliable way to locate package resources.
Suggested change
| package_root = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) | |
| is_developer = os.path.isdir(os.path.join(package_root, 'tests')) | |
| copy = not is_developer | |
| if not is_developer: | |
| package_root = None | |
| import importlib.resources | |
| try: | |
| # Try to get the package root using importlib.resources | |
| package_root = importlib.resources.files("jtop").parent | |
| package_root = str(package_root) | |
| except Exception: | |
| # Fallback to previous method if importlib.resources fails | |
| package_root = os.path.dirname(os.path.dirname(os.path.realpath(__file__))) | |
| is_developer = os.path.isdir(os.path.join(package_root, 'tests')) | |
| copy = not is_developer | |
| if not is_developer: | |
| package_root = None |
Comment on lines
+166
to
+168
| igpu[name] = {'type': 'integrated', 'path': path, 'frq_path': frq_path} | ||
| logger.info(f"GPU \"{name}\" status in {path}") | ||
| logger.info(f"GPU \"{name}\" frq in {frq_path}") |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Using the original device name as the dictionary key may cause inconsistencies.
Using unnormalized device names as keys can result in inconsistent dictionary access due to differences in case or whitespace. Use normalized_name to ensure reliable lookups.
Suggested change
| igpu[name] = {'type': 'integrated', 'path': path, 'frq_path': frq_path} | |
| logger.info(f"GPU \"{name}\" status in {path}") | |
| logger.info(f"GPU \"{name}\" frq in {frq_path}") | |
| normalized_name = name.strip().lower().replace(" ", "_") | |
| igpu[normalized_name] = {'type': 'integrated', 'path': path, 'frq_path': frq_path} | |
| logger.info(f"GPU \"{normalized_name}\" status in {path}") | |
| logger.info(f"GPU \"{normalized_name}\" frq in {frq_path}") |
| args = parser.parse_args() | ||
| # Install jtop service if requested | ||
| if args.install_service: | ||
| if os.geteuid() != 0: |
Contributor
There was a problem hiding this comment.
issue (code-quality): Extract code out into function [×2] (extract-method)
|
|
||
|
|
||
| def _variables_template_path(package_root, name): | ||
| candidates = [] |
Contributor
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional [×3] (
use-named-expression) - Replace a for append loop with list extend (
for-append-to-extend) - Merge consecutive list appends into a single extend (
merge-list-appends-into-extend)
|
|
||
|
|
||
| def _service_template_path(package_root, name): | ||
| candidates = [] |
Contributor
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional [×3] (
use-named-expression) - Replace a for append loop with list extend (
for-append-to-extend) - Merge consecutive list appends into a single extend (
merge-list-appends-into-extend)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Provide a robust mechanism for locating and installing service and environment variable templates, introduce a new install-service CLI flag, refine GPU detection logic, and update service-related error messages to use the new flag.
New Features:
Enhancements: