Conversation
rbx
changed the title
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds comprehensive static typing across the codebase: annotated public APIs, refactored Prices to be stateful, introduced a typed MetricsTracker, and updated job/backlog and plotting call-sites to use typed signatures. No major behavioral logic rewrites reported; most edits are signature/typing, small API renames, and test call-site updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/sampler_jobs.py (2)
36-45:⚠️ Potential issue | 🟠 MajorFix bin stepping to honor
bin_minutes(multi‑hour bins are mis-binned).Line 41–45 sets
time_deltato 1 hour (or 1 day) even whenbin_minutesis 120, 180, 2880, etc. This generates period keys at the wrong cadence and inserts extra empty bins, which breaks deterministic binning. Usebin_minutesconsistently fortime_delta, and make the format reflect minutes when needed.✅ Suggested fix
- if bin_minutes >= 1440: # Daily or longer (≥ 24 hours) - time_format = "%Y-%m-%d" - time_delta = datetime.timedelta(days=1) - elif bin_minutes >= 60: # Hourly or longer - time_format = "%Y-%m-%d %H:00" - time_delta = datetime.timedelta(hours=1) - else: # Less than an hour - time_format = "%Y-%m-%d %H:%M" - time_delta = datetime.timedelta(minutes=bin_minutes) + if bin_minutes >= 1440 and bin_minutes % 1440 == 0: # Daily-aligned bins + time_format = "%Y-%m-%d" + elif bin_minutes >= 60 and bin_minutes % 60 == 0: # Hour-aligned bins + time_format = "%Y-%m-%d %H:00" + else: # Minute-aligned bins + time_format = "%Y-%m-%d %H:%M" + time_delta = datetime.timedelta(minutes=bin_minutes)As per coding guidelines: Jobs sampler (src/sampler_jobs.py) must parse Slurm logs, bin jobs by time period (default hourly), and support aggregation that groups similar jobs by (nodes, cores, duration) while converting sub-hour jobs to hourly equivalents by adjusting resource requirements to preserve core-hours.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sampler_jobs.py` around lines 36 - 45, The bin stepping currently hardcodes 1-hour/1-day deltas; change the logic in the time_format/time_delta assignment (the block that sets time_format and time_delta) to compute time_delta from bin_minutes (time_delta = datetime.timedelta(minutes=bin_minutes)) and choose the string format based on divisibility: if bin_minutes % 1440 == 0 use "%Y-%m-%d", elif bin_minutes % 60 == 0 use "%Y-%m-%d %H:00", else use "%Y-%m-%d %H:%M"; update the code that builds period keys to use this computed time_delta so multi-hour and multi-day bins advance correctly and avoid extra empty bins.
326-346:⚠️ Potential issue | 🟠 MajorPreserve core-hours when converting sub-hour jobs (flooring loses work).
Line 330 floors
total_core_hoursviaint(...), which undercounts core-hours (e.g., 89 core‑minutes → 1 core‑hour). That violates the requirement to preserve core-hours when converting sub-hour jobs. At minimum, avoid truncation and carry the exact core‑minutes forward for downstream accounting.✅ Suggested fix
- equivalent_cores = max(1, int(total_core_hours)) + equivalent_cores = max(1, math.ceil(total_core_hours)) ... - hourly_job: Job = { + hourly_job: Job = { 'nnodes': equivalent_nodes, 'cores_per_node': cores_per_node_needed, 'duration_hours': 1, # 1 hour + 'total_core_minutes': total_core_minutes, 'original_job_count': agg_job['count'] }As per coding guidelines: Jobs sampler (src/sampler_jobs.py) must parse Slurm logs, bin jobs by time period (default hourly), and support aggregation that groups similar jobs by (nodes, cores, duration) while converting sub-hour jobs to hourly equivalents by adjusting resource requirements to preserve core-hours.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sampler_jobs.py` around lines 326 - 346, The code truncates total core-hours by using int(total_core_hours) to set equivalent_cores, which loses fractional core-hours; change equivalent_cores to use the full float (e.g., equivalent_cores = max(1.0, total_core_hours)) so you don't undercount, keep using math.ceil when converting cores to integer nodes/cores per node (equivalent_nodes = max(1, math.ceil(equivalent_cores / cores_per_node)) and cores_per_node_needed = min(cores_per_node, math.ceil(cores_needed / equivalent_nodes))) and add the precise total_core_hours value into the created hourly_job (for example as 'total_core_hours' or 'core_hours') so downstream accounting can preserve exact core-hours while allocation remains integer.src/prices_deterministic.py (1)
1-12:⚠️ Potential issue | 🟡 MinorAdd
from __future__ import annotationsfor consistency with other files in the project.While this file uses PEP 604 union syntax (
X | Y) in type annotations and does not havefrom __future__ import annotations, this is not a runtime issue since the project targets Python 3.10+ (confirmed in.github/workflows/tests.yml). However, other files in the codebase—such assrc/sampler_duration.py,src/plotter.py, andsrc/workloadgen_cli.py—consistently include this import. Adding it here maintains uniformity across the codebase.Proposed fix
+from __future__ import annotations + import numpy as np import matplotlib.pyplot as plt from collections import deque🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prices_deterministic.py` around lines 1 - 12, Add the line "from __future__ import annotations" at the top of this module before any other imports to match other files and ensure forward-compatible, deferred evaluation of type annotations used in the Prices class and its __init__ signature (which uses PEP 604 union syntax like np.ndarray | list[float] | None); no other code changes are needed.src/baseline.py (1)
33-54:⚠️ Potential issue | 🟡 MinorDocstring is missing the
baseline_backlog_queueparameter.The
Argssection documents parameters up toenv_printbut omitsbaseline_backlog_queue: deque, which was added to the signature on line 31.📝 Proposed fix
metrics: MetricsTracker object to update with baseline job metrics env_print: Print function for logging + baseline_backlog_queue: Backlog queue for overflow jobs Returns:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/baseline.py` around lines 33 - 54, Update the function docstring for the baseline simulation (the triple-quoted block in baseline.py above the function signature that includes baseline_backlog_queue) to document the missing parameter baseline_backlog_queue: deque; add a brief description like "baseline_backlog_queue: deque of jobs waiting for scheduling (holds job IDs or job dicts)" alongside the other Args entries and keep the same formatting style as the existing Args lines so the docstring accurately reflects the function signature and parameters such as baseline_state, baseline_cores_available, baseline_running_jobs, current_price, new_jobs_count, new_jobs_durations, new_jobs_nodes, new_jobs_cores, baseline_next_empty_slot, next_job_id, metrics, env_print.
🧹 Nitpick comments (7)
src/sampler_hourly.py (2)
22-22: Nit:return None→return.For a
-> Nonefunction, barereturnis the idiomatic form.♻️ Proposed fix
- return None + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sampler_hourly.py` at line 22, Replace the non-idiomatic "return None" with a bare "return" in the function that is annotated with "-> None" (the function containing the current "return None" at line 22) so the function uses the idiomatic form for returning from a None-annotated function; update the single "return None" statement to "return" in that function (no other logic changes).
142-147:get_stats()return-type annotation is inaccurate for numpy scalars.The inner dict values include
np.mean(...)→np.floating[Any],np.max(...)→np.intp, andlen(...)→int. None of these satisfyfloatin numpy's type stubs — strict type checkers (mypy/Pylance with numpy stubs) will flag the annotationdict[int, dict[str, float]]as a type error.Although
np.float_is a subtype of Python's native float at runtime, type checkers like Pylance report"float_" is incompatible with "float"when numpy stubs are active. Thenp.intpreturn ofnp.maxand theintreturn oflencompound the problem further.Consider loosening the inner dict's value type to
Any(consistent with howhourly_templatesis typed elsewhere in this class), or use a more precise union:♻️ Proposed fix
- def get_stats(self) -> dict[int, dict[str, float]]: + def get_stats(self) -> dict[int, dict[str, Any]]: """Return summary statistics of the sampler.""" if not self.initialized: raise RuntimeError("Sampler not initialized. Call parse_jobs() first.") - stats: dict[int, dict[str, float]] = {} + stats: dict[int, dict[str, Any]] = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sampler_hourly.py` around lines 142 - 147, The return-type annotation of get_stats is too strict for numpy scalar results; change the signature to return dict[int, dict[str, Any]] (import Any from typing) or otherwise widen the inner-value type to accept numpy scalars (e.g., Union[int, float, np.floating, np.integer]) so mypy/Pylance stop flagging np.mean/np.max/len returns; update the get_stats function annotation and any related usages to use the same relaxed type (consistent with hourly_templates typing).src/plotter.py (1)
88-88: Consider prefixing the unusedepisode_costsparameter to silence the linter.Ruff flags this as unused (ARG001). Since it's kept for backward compatibility (documented in the docstring), prefixing with
_would communicate intent and silence the warning.Suggested fix
-def plot_dashboard(env: ComputeClusterEnv, num_hours: int, max_nodes: int, episode_costs: list[dict[str, float | int]] | None = None, save: bool = True, show: bool = True, suffix: int | str = "") -> None: +def plot_dashboard(env: ComputeClusterEnv, num_hours: int, max_nodes: int, _episode_costs: list[dict[str, float | int]] | None = None, save: bool = True, show: bool = True, suffix: int | str = "") -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plotter.py` at line 88, The parameter episode_costs in function plot_dashboard is currently unused and triggers Ruff ARG001; rename it to _episode_costs in the plot_dashboard signature to indicate it's intentionally unused (keeping its docstring note for backward compatibility) and update any internal references to use the new name if present; no other behavior changes should be made so callers remain compatible with the parameter name while the linter warning is silenced.src/sampler_duration.py (1)
62-76: Stale docstring contradicts the return type annotation.The docstring says "Single float if n=1, otherwise array of floats" but the method always returns an
NDArray[np.intp](via.astype(int)), matching the type hint. Consider updating the docstring to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sampler_duration.py` around lines 62 - 76, The docstring for sample() is stale and describes floats/scalar behavior that doesn't match the annotated return type NDArray[np.intp] and the actual implementation (which always returns a numpy array cast to int via .astype(int)); update the docstring in sample() to state it returns a numpy.ndarray of integers (NDArray[np.intp]) with shape (n,) and that when n==1 it yields a single-element array rather than a plain float/scalar; ensure the Args/Returns section mentions integers and the array shape to match sample_values/probabilities usage.src/workloadgen_cli.py (1)
22-31: Type-checker note:tuple()from a generator yieldstuple[float, ...], not the fixed-length tuple.The return type says
tuple[float, float, float, float]buttuple(float(p) for p in parts)producestuple[float, ...]as far as type checkers are concerned. Same pattern onparse_quad_ints(line 41) andparse_quad_ranges(line 65). Runtime behavior is correct since validation ensures exactly 4 elements.If you want strict type-checker compliance, you could unpack explicitly, though this is entirely optional:
Example for parse_quad_floats
try: - return tuple(float(p) for p in parts) + a, b, c, d = (float(p) for p in parts) + return (a, b, c, d) except ValueError as exc:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workloadgen_cli.py` around lines 22 - 31, The type error arises because using tuple(generator) yields tuple[float, ...] for parse_quad_floats (and similarly for parse_quad_ints and parse_quad_ranges); to satisfy the declared fixed-length return types, explicitly convert parts to the target types, unpack into four named variables (e.g., a,b,c,d) after validating len(parts)==4, then return (a, b, c, d) so the type checker sees a fixed 4-tuple rather than a variadic tuple.src/reward_calculation.py (1)
181-181:env_printtyped asCallable[[str], None]— inconsistent withCallable[..., None]inbaseline.py.Here
env_printis typedCallable[[str], None](exactly onestrargument), but insrc/baseline.pyline 30 the same callback is typedCallable[..., None]. The actual method signature isenv_print(self, *args: Any) -> None. UsingCallable[..., None]consistently would be more accurate and avoid strict type-checker complaints about passing a variadic function where a single-argument callable is expected.Proposed fix
- env_print: Callable[[str], None]) -> tuple[float, float, float, float, float, float]: + env_print: Callable[..., None]) -> tuple[float, float, float, float, float, float]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reward_calculation.py` at line 181, The env_print parameter in the function signature is currently typed as Callable[[str], None] which conflicts with the variadic env_print used elsewhere (e.g., the env_print method in baseline.py typed as Callable[..., None]); change the type annotation for env_print in the function (the signature that returns tuple[float, float, float, float, float, float]) from Callable[[str], None] to Callable[..., None] so callers passing env_print(self, *args: Any) match the type; update only the type annotation for the env_print parameter (and any similar signatures in this module) to Callable[..., None].src/environment.py (1)
49-50: Nit:renderparameter name shadows Gymnasium convention.Gymnasium's
rendermethod signature isrender(self) -> RenderFrame | list[RenderFrame] | None(nomodeparameter — the mode is set viarender_modeat construction). This implementation usesrenderto mutateself.render_modeat runtime, which deviates from the Gym API. This is pre-existing behavior, not introduced by this PR, but the new annotation surfaces the deviation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/environment.py` around lines 49 - 50, The render method currently accepts a mode parameter and mutates self.render_mode, which diverges from Gymnasium conventions; change the signature of render to no-argument (def render(self):) and remove any in-method mutation of self.render_mode in the render method in Environment (or the class containing render), and instead provide a separate setter (e.g., set_render_mode or a render_mode property setter) or require render_mode to be set at construction so that render only performs rendering and follows Gymnasium's API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/environment.py`:
- Line 74: The constructor parameter workload_gen is incorrectly typed as
WorkloadGenConfig; change its annotation to WorkloadGenerator to match actual
callers and usage (e.g. generate_jobs expects WorkloadGenerator). Update the
signature that contains "workload_gen: WorkloadGenConfig | None" to
"workload_gen: WorkloadGenerator | None" and ensure the module imports
WorkloadGenerator from src.workload_generator (or the correct symbol) instead
of/alongside WorkloadGenConfig so type checking and callers like train.py and
test_sanity_env.py align.
In `@src/plot.py`:
- Line 14: The plot function's signature allows suffix to be a str or empty
string but the code formats it with {suffix:09d}, which requires an integer;
update the plot function signature to use an integer type and a sensible integer
default (e.g., change suffix: int | str = "" to suffix: int = 0) so runtime
ValueError is avoided, and ensure any callers (e.g., places passing
self.current_step) continue to pass integers consistently.
In `@src/sampler_hourly.py`:
- Line 33: Remove the dead local variable hourly_job_counts from parse_jobs:
delete the declaration "hourly_job_counts: defaultdict[int, list[int]] =
defaultdict(list)" and, if defaultdict is now unused in the module, remove its
import as well to avoid unused-import lints; ensure no other references to
hourly_job_counts exist before deleting.
In `@src/workload_generator.py`:
- Line 29: The declared return type tuple[int, list[int], list[int], list[int]]
is wrong for code paths where new_jobs_durations is filled from numpy arrays;
change the code that assigns from durations_sampler.sample() and
np_random.integers(...) to convert those NDArray results to Python lists (call
.tolist()) so new_jobs_durations and the related arrays match list[int], leaving
the append-based paths unchanged; specifically update the assignments that call
durations_sampler.sample() and np_random.integers(...) to use .tolist() so the
function signature can remain as declared.
---
Outside diff comments:
In `@src/baseline.py`:
- Around line 33-54: Update the function docstring for the baseline simulation
(the triple-quoted block in baseline.py above the function signature that
includes baseline_backlog_queue) to document the missing parameter
baseline_backlog_queue: deque; add a brief description like
"baseline_backlog_queue: deque of jobs waiting for scheduling (holds job IDs or
job dicts)" alongside the other Args entries and keep the same formatting style
as the existing Args lines so the docstring accurately reflects the function
signature and parameters such as baseline_state, baseline_cores_available,
baseline_running_jobs, current_price, new_jobs_count, new_jobs_durations,
new_jobs_nodes, new_jobs_cores, baseline_next_empty_slot, next_job_id, metrics,
env_print.
In `@src/prices_deterministic.py`:
- Around line 1-12: Add the line "from __future__ import annotations" at the top
of this module before any other imports to match other files and ensure
forward-compatible, deferred evaluation of type annotations used in the Prices
class and its __init__ signature (which uses PEP 604 union syntax like
np.ndarray | list[float] | None); no other code changes are needed.
In `@src/sampler_jobs.py`:
- Around line 36-45: The bin stepping currently hardcodes 1-hour/1-day deltas;
change the logic in the time_format/time_delta assignment (the block that sets
time_format and time_delta) to compute time_delta from bin_minutes (time_delta =
datetime.timedelta(minutes=bin_minutes)) and choose the string format based on
divisibility: if bin_minutes % 1440 == 0 use "%Y-%m-%d", elif bin_minutes % 60
== 0 use "%Y-%m-%d %H:00", else use "%Y-%m-%d %H:%M"; update the code that
builds period keys to use this computed time_delta so multi-hour and multi-day
bins advance correctly and avoid extra empty bins.
- Around line 326-346: The code truncates total core-hours by using
int(total_core_hours) to set equivalent_cores, which loses fractional
core-hours; change equivalent_cores to use the full float (e.g.,
equivalent_cores = max(1.0, total_core_hours)) so you don't undercount, keep
using math.ceil when converting cores to integer nodes/cores per node
(equivalent_nodes = max(1, math.ceil(equivalent_cores / cores_per_node)) and
cores_per_node_needed = min(cores_per_node, math.ceil(cores_needed /
equivalent_nodes))) and add the precise total_core_hours value into the created
hourly_job (for example as 'total_core_hours' or 'core_hours') so downstream
accounting can preserve exact core-hours while allocation remains integer.
---
Nitpick comments:
In `@src/environment.py`:
- Around line 49-50: The render method currently accepts a mode parameter and
mutates self.render_mode, which diverges from Gymnasium conventions; change the
signature of render to no-argument (def render(self):) and remove any in-method
mutation of self.render_mode in the render method in Environment (or the class
containing render), and instead provide a separate setter (e.g., set_render_mode
or a render_mode property setter) or require render_mode to be set at
construction so that render only performs rendering and follows Gymnasium's API.
In `@src/plotter.py`:
- Line 88: The parameter episode_costs in function plot_dashboard is currently
unused and triggers Ruff ARG001; rename it to _episode_costs in the
plot_dashboard signature to indicate it's intentionally unused (keeping its
docstring note for backward compatibility) and update any internal references to
use the new name if present; no other behavior changes should be made so callers
remain compatible with the parameter name while the linter warning is silenced.
In `@src/reward_calculation.py`:
- Line 181: The env_print parameter in the function signature is currently typed
as Callable[[str], None] which conflicts with the variadic env_print used
elsewhere (e.g., the env_print method in baseline.py typed as Callable[...,
None]); change the type annotation for env_print in the function (the signature
that returns tuple[float, float, float, float, float, float]) from
Callable[[str], None] to Callable[..., None] so callers passing env_print(self,
*args: Any) match the type; update only the type annotation for the env_print
parameter (and any similar signatures in this module) to Callable[..., None].
In `@src/sampler_duration.py`:
- Around line 62-76: The docstring for sample() is stale and describes
floats/scalar behavior that doesn't match the annotated return type
NDArray[np.intp] and the actual implementation (which always returns a numpy
array cast to int via .astype(int)); update the docstring in sample() to state
it returns a numpy.ndarray of integers (NDArray[np.intp]) with shape (n,) and
that when n==1 it yields a single-element array rather than a plain
float/scalar; ensure the Args/Returns section mentions integers and the array
shape to match sample_values/probabilities usage.
In `@src/sampler_hourly.py`:
- Line 22: Replace the non-idiomatic "return None" with a bare "return" in the
function that is annotated with "-> None" (the function containing the current
"return None" at line 22) so the function uses the idiomatic form for returning
from a None-annotated function; update the single "return None" statement to
"return" in that function (no other logic changes).
- Around line 142-147: The return-type annotation of get_stats is too strict for
numpy scalar results; change the signature to return dict[int, dict[str, Any]]
(import Any from typing) or otherwise widen the inner-value type to accept numpy
scalars (e.g., Union[int, float, np.floating, np.integer]) so mypy/Pylance stop
flagging np.mean/np.max/len returns; update the get_stats function annotation
and any related usages to use the same relaxed type (consistent with
hourly_templates typing).
In `@src/workloadgen_cli.py`:
- Around line 22-31: The type error arises because using tuple(generator) yields
tuple[float, ...] for parse_quad_floats (and similarly for parse_quad_ints and
parse_quad_ranges); to satisfy the declared fixed-length return types,
explicitly convert parts to the target types, unpack into four named variables
(e.g., a,b,c,d) after validating len(parts)==4, then return (a, b, c, d) so the
type checker sees a fixed 4-tuple rather than a variadic tuple.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/baseline.pysrc/callbacks.pysrc/config.pysrc/environment.pysrc/job_management.pysrc/metrics_tracker.pysrc/node_management.pysrc/plot.pysrc/plotter.pysrc/prices.pysrc/prices_deterministic.pysrc/reward_calculation.pysrc/sampler_duration.pysrc/sampler_hourly.pysrc/sampler_jobs.pysrc/weights.pysrc/workload_generator.pysrc/workloadgen.pysrc/workloadgen_cli.py
💤 Files with no reviewable changes (1)
- src/prices.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/workload_generator.py (1)
52-55:⚠️ Potential issue | 🟠 MajorGuard optional
jobs_samplerbefore use.At Line 54,
jobs_sampleris optional in the signature but used unconditionally. Ifexternal_jobsis configured and sampler wiring is missing, this fails withAttributeErrorinstead of a controlled error.Proposed fix
if external_jobs and not workload_gen: + if jobs_sampler is None: + raise RuntimeError("jobs_sampler is required when external_jobs is enabled.") # Use jobs sampler for pattern-based replay jobs = jobs_sampler.sample_one_hourly(wrap=True)["hourly_jobs"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/workload_generator.py` around lines 52 - 55, The code uses jobs_sampler unconditionally in the external-jobs branch (when external_jobs and not workload_gen) by calling jobs_sampler.sample_one_hourly; guard the optional jobs_sampler (e.g., check jobs_sampler is not None) before calling sample_one_hourly and if it is missing raise a clear, controlled exception (ValueError or custom) explaining that jobs_sampler must be provided when external_jobs is enabled, or alternatively fall back to an appropriate default behavior; update the logic around jobs_sampler.sample_one_hourly and the jobs assignment to only execute when jobs_sampler is present.
🧹 Nitpick comments (4)
src/environment.py (1)
268-268: Minor: unparameterizeddictinresetreturn type.
stepconsistently usesdict[str, Any]for the info dict;resetshould too.♻️ Proposed fix
- def reset(self, seed: int | None = None, options: dict[str, Any] | None = None) -> tuple[dict[str, np.ndarray], dict]: + def reset(self, seed: int | None = None, options: dict[str, Any] | None = None) -> tuple[dict[str, np.ndarray], dict[str, Any]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/environment.py` at line 268, The reset method's return type uses an unparameterized dict for the info return; update the signature of reset (def reset) to return tuple[dict[str, np.ndarray], dict[str, Any]] to match step's info typing. Locate the reset function declaration and change its second element of the tuple return annotation from dict to dict[str, Any], ensuring imports for Any are present if needed and that any docstring/type hints align with the new annotation.src/metrics_tracker.py (1)
128-137: Inconsistent guard style:if self.episode_jobs_submittedvs> 0used elsewhere.Lines 130 and 135 use a bare truthiness test while lines 119, 124, and the rest of the method use
> 0. Both are equivalent forint, but aligning them improves readability.♻️ Proposed fix
- drop_rate: float = ( - (self.episode_jobs_dropped / self.episode_jobs_submitted * 100) - if self.episode_jobs_submitted - else 0.0 - ) - baseline_drop_rate: float = ( - (self.episode_baseline_jobs_dropped / self.episode_baseline_jobs_submitted * 100) - if self.episode_baseline_jobs_submitted - else 0.0 - ) + drop_rate: float = ( + (self.episode_jobs_dropped / self.episode_jobs_submitted * 100) + if self.episode_jobs_submitted > 0 + else 0.0 + ) + baseline_drop_rate: float = ( + (self.episode_baseline_jobs_dropped / self.episode_baseline_jobs_submitted * 100) + if self.episode_baseline_jobs_submitted > 0 + else 0.0 + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/metrics_tracker.py` around lines 128 - 137, The guard expressions for computing drop_rate and baseline_drop_rate use bare truthiness checks; change the conditions to explicit comparisons (use > 0) to match the rest of the method. Update the ternary guards in the drop_rate calculation (referencing drop_rate and self.episode_jobs_submitted) and in the baseline_drop_rate calculation (referencing baseline_drop_rate and self.episode_baseline_jobs_submitted) to use "if self.episode_jobs_submitted > 0" and "if self.episode_baseline_jobs_submitted > 0" respectively so the style is consistent.src/job_management.py (1)
13-13: Parameterizedequetype hint for precision.All three signatures declare
backlog_queue: deque(unparameterized). Queue items arelist[int](4-element[duration, age, nodes, cores]); annotating asdeque[list[int]]makes the element type explicit and catches misuse earlier under strict type-checkers.♻️ Proposed refactor
-def age_backlog_queue(backlog_queue: deque, _metrics: MetricsTracker, _is_baseline: bool = False) -> int: +def age_backlog_queue(backlog_queue: deque[list[int]], _metrics: MetricsTracker, _is_baseline: bool = False) -> int:-def fill_queue_from_backlog(job_queue_2d: np.ndarray, backlog_queue: deque, next_empty_slot: int) -> tuple[int, int]: +def fill_queue_from_backlog(job_queue_2d: np.ndarray, backlog_queue: deque[list[int]], next_empty_slot: int) -> tuple[int, int]:- backlog_queue: deque | None = None, + backlog_queue: deque[list[int]] | None = None,Also applies to: 47-47, 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/job_management.py` at line 13, Update the unparameterized deque type annotations to be precise: change occurrences like backlog_queue: deque to backlog_queue: deque[list[int]] (items are 4-element lists of ints: [duration, age, nodes, cores]) so static type checkers catch misuse; specifically update the signature of age_backlog_queue(backlog_queue: deque, ...) to backlog_queue: deque[list[int]] and make the same replacement for the other functions in this file that currently use backlog_queue: deque.src/reward_calculation.py (1)
79-88: Remove dead code:_reward_efficiency_normalizedis unreachable and its bounds are unused.
_reward_efficiency_normalizedis never called. Thecalculate()method uses_reward_energy_efficiency_normalized()+_blackout_term()instead (line 205). The efficiency bounds_min_efficiency_rewardand_max_efficiency_rewardare only referenced within this dead method, making them unreachable from all live code paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reward_calculation.py` around lines 79 - 88, The _reward_efficiency_normalized method and its associated bounds (_min_efficiency_reward, _max_efficiency_reward) are dead code: remove the _reward_efficiency_normalized function and eliminate the unused attributes/variables _min_efficiency_reward and _max_efficiency_reward from the class (and from __init__ or any defaults) so they are not kept around; ensure no remaining references to _reward_efficiency_normalized, _min_efficiency_reward or _max_efficiency_reward remain (search for those symbols) and run tests to confirm behavior still relies on _reward_energy_efficiency_normalized() + _blackout_term().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/prices_deterministic.py`:
- Line 19: predicted_prices is declared as np.ndarray | None but used
unconditionally in get_price_context (np.mean(self.predicted_prices)), causing
type-checker errors; fix by either narrowing the attribute type to np.ndarray
and initializing it (e.g., assign an empty np.ndarray placeholder) before
reset() in __init__ so reset can overwrite it, or keep the union type and add a
None-guard in get_price_context (raise/assert or return a sensible default)
before calling np.mean; update the annotation on self.predicted_prices and
modify __init__/reset or add the guard in get_price_context accordingly,
referencing self.predicted_prices, __init__, reset, and get_price_context.
In `@src/reward_calculation.py`:
- Around line 178-181: The calculate method currently accepts num_node_changes
but never uses it; either remove the unused parameter from the calculate
signature (and update any callers) or keep it as a planned feature by adding a
local noqa to silence ARG002 and a short TODO; specifically, if you intend to
implement node-change penalties later, keep num_node_changes in def
calculate(...) and add a comment like "# noqa: ARG002 TODO: apply node-change
penalty when implementing transition costs" next to the parameter or at the top
of the function; otherwise delete num_node_changes from the calculate signature
and adjust all callers (and any type hints) accordingly.
---
Outside diff comments:
In `@src/workload_generator.py`:
- Around line 52-55: The code uses jobs_sampler unconditionally in the
external-jobs branch (when external_jobs and not workload_gen) by calling
jobs_sampler.sample_one_hourly; guard the optional jobs_sampler (e.g., check
jobs_sampler is not None) before calling sample_one_hourly and if it is missing
raise a clear, controlled exception (ValueError or custom) explaining that
jobs_sampler must be provided when external_jobs is enabled, or alternatively
fall back to an appropriate default behavior; update the logic around
jobs_sampler.sample_one_hourly and the jobs assignment to only execute when
jobs_sampler is present.
---
Nitpick comments:
In `@src/environment.py`:
- Line 268: The reset method's return type uses an unparameterized dict for the
info return; update the signature of reset (def reset) to return tuple[dict[str,
np.ndarray], dict[str, Any]] to match step's info typing. Locate the reset
function declaration and change its second element of the tuple return
annotation from dict to dict[str, Any], ensuring imports for Any are present if
needed and that any docstring/type hints align with the new annotation.
In `@src/job_management.py`:
- Line 13: Update the unparameterized deque type annotations to be precise:
change occurrences like backlog_queue: deque to backlog_queue: deque[list[int]]
(items are 4-element lists of ints: [duration, age, nodes, cores]) so static
type checkers catch misuse; specifically update the signature of
age_backlog_queue(backlog_queue: deque, ...) to backlog_queue: deque[list[int]]
and make the same replacement for the other functions in this file that
currently use backlog_queue: deque.
In `@src/metrics_tracker.py`:
- Around line 128-137: The guard expressions for computing drop_rate and
baseline_drop_rate use bare truthiness checks; change the conditions to explicit
comparisons (use > 0) to match the rest of the method. Update the ternary guards
in the drop_rate calculation (referencing drop_rate and
self.episode_jobs_submitted) and in the baseline_drop_rate calculation
(referencing baseline_drop_rate and self.episode_baseline_jobs_submitted) to use
"if self.episode_jobs_submitted > 0" and "if
self.episode_baseline_jobs_submitted > 0" respectively so the style is
consistent.
In `@src/reward_calculation.py`:
- Around line 79-88: The _reward_efficiency_normalized method and its associated
bounds (_min_efficiency_reward, _max_efficiency_reward) are dead code: remove
the _reward_efficiency_normalized function and eliminate the unused
attributes/variables _min_efficiency_reward and _max_efficiency_reward from the
class (and from __init__ or any defaults) so they are not kept around; ensure no
remaining references to _reward_efficiency_normalized, _min_efficiency_reward or
_max_efficiency_reward remain (search for those symbols) and run tests to
confirm behavior still relies on _reward_energy_efficiency_normalized() +
_blackout_term().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
src/baseline.pysrc/callbacks.pysrc/config.pysrc/environment.pysrc/job_management.pysrc/metrics_tracker.pysrc/node_management.pysrc/plot.pysrc/plotter.pysrc/prices_deterministic.pysrc/reward_calculation.pysrc/sampler_duration.pysrc/sampler_hourly.pysrc/sampler_jobs.pysrc/weights.pysrc/workload_generator.pysrc/workloadgen.pysrc/workloadgen_cli.pytest/test_plot_dummy.pytrain.py
💤 Files with no reviewable changes (2)
- train.py
- test/test_plot_dummy.py
✅ Files skipped from review due to trivial changes (1)
- src/callbacks.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/weights.py
- src/node_management.py
- src/sampler_jobs.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/reward_calculation.py (1)
178-181:noqa: ARG002onnum_node_changesnow correctly suppresses the unused-argument warning.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reward_calculation.py` around lines 178 - 181, The calculate method declares num_node_changes as a reserved-but-unused parameter and you should either keep the existing "# noqa: ARG002" comment on that parameter (ensure it's exactly after num_node_changes in the signature) or explicitly consume it to silence linters by adding a short no-op assignment in the function body (e.g., _ = num_node_changes) inside calculate; reference the calculate(...) signature and the num_node_changes parameter when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/prices_deterministic.py`:
- Around line 5-11: The PR deleted src/prices.py which violates the guideline
that pricing models live in both src/prices.py and src/prices_deterministic.py;
restore compliance by either re-adding a minimal src/prices.py that imports and
re-exports the deterministic implementation (e.g., from src.prices_deterministic
import Prices and expose Prices) or update the project guideline to state that
src/prices_deterministic.py is now the canonical implementation; ensure the
restored src/prices.py references the Prices class from prices_deterministic.py
so consumers still import from src.prices while keeping the existing Prices
class intact.
In `@src/reward_calculation.py`:
- Around line 116-120: Wrap the np.clip return values with float(...) to match
the -> float annotations: change the return in _penalty_idle_normalized to
return float(np.clip(...)), do the same in _penalty_job_age_normalized, and
ensure the branch in _reward_efficiency_normalized that currently returns an
unwrapped np.clip is also wrapped with float(...); keep other branches that
already return Python floats unchanged so all code paths return a Python float.
---
Duplicate comments:
In `@src/reward_calculation.py`:
- Around line 178-181: The calculate method declares num_node_changes as a
reserved-but-unused parameter and you should either keep the existing "# noqa:
ARG002" comment on that parameter (ensure it's exactly after num_node_changes in
the signature) or explicitly consume it to silence linters by adding a short
no-op assignment in the function body (e.g., _ = num_node_changes) inside
calculate; reference the calculate(...) signature and the num_node_changes
parameter when making the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 24: The documentation change removed the documented core pricing module
reference to prices_deterministic.py while the repository rule still requires
both pricing modules; either restore the missing mention or update the rule.
Revert or re-add the entry for prices_deterministic.py in CLAUDE.md so both
prices.py and prices_deterministic.py are listed under pricing, or if
consolidation is intentional, update the repository guideline text to reflect a
single prices.py implementation; ensure the change references the module names
prices.py and prices_deterministic.py and the CLAUDE.md docs section
consistently.
In `@src/environment.py`:
- Line 67: ComputeClusterEnv.__init__ currently accepts external_prices: str |
None and passes it directly to Prices, but Prices expects numeric sequences
(np.ndarray | list[float] | None); update ComputeClusterEnv.__init__ to detect
when external_prices is a str (file path) and load/parse it into a numeric
sequence (e.g., use numpy.loadtxt or json/CSV parsing to produce a list[float]
or np.ndarray) before constructing Prices, validate the parsed data is numeric
(or raise a clear ValueError), and otherwise accept None or already-numeric
inputs unchanged; ensure you reference ComputeClusterEnv.__init__ and Prices
when making the change so callers can still pass arrays or file paths safely.
In `@src/prices.py`:
- Around line 71-76: The synthetic-mode branch of reset ignores the start_index
parameter: update the synthetic reset logic in the reset(start_index=...) path
so self.price_index is initialized to start_index (not always to
self.PREDICTION_WINDOW) and build self.predicted_prices by calling
self._synthetic_price_at(i) for i in range(start_index, start_index +
self.PREDICTION_WINDOW) (or an equivalent offset loop) while preserving
dtype=np.float32; ensure references to self.PREDICTION_WINDOW,
self._synthetic_price_at, self.price_index, and self.predicted_prices are used
so synthetic timeline positioning matches external mode.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.mdsrc/environment.pysrc/prices.pysrc/reward_calculation.pytest/test_price_history.pytest/test_prices_cycling.py
✅ Files skipped from review due to trivial changes (1)
- test/test_price_history.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/prices.py (1)
71-77:start_indexis now correctly applied in synthetic mode.The previous review flagged that
start_indexwas ignored in the synthetic branch. The current code correctly usesstart(derived fromstart_index) for bothself.price_indexand the_synthetic_price_atcalls. This is resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prices.py` around lines 71 - 77, The synthetic branch now correctly applies start_index by setting start = int(start_index), updating self.price_index, and generating self.predicted_prices via self._synthetic_price_at over self.PREDICTION_WINDOW; no code changes required—just remove the duplicate/obsolete review comment and mark the change approved so the PR can be merged (references: start, self.price_index, self.predicted_prices, _synthetic_price_at, PREDICTION_WINDOW).src/environment.py (3)
67-67: Previously flaggedexternal_prices: str | Nonetype mismatch is now resolved.The parameter is correctly typed as
list[float] | np.ndarray | None, which is consistent with thePricesconstructor signature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/environment.py` at line 67, The type mismatch for the parameter external_prices has been corrected to list[float] | np.ndarray | None to match the Prices constructor signature; no further code changes are necessary—verify the signature in the Prices class and ensure any callers passing external_prices conform to the updated type.
63-75:⚠️ Potential issue | 🟠 Major
X | Yunion syntax in__init__signature requires Python ≥ 3.10.Same concern as flagged in
src/prices.py: thelist[float] | np.ndarray | Noneannotation on Line 67 andWorkloadGenerator | Noneon Line 74 will raiseTypeErrorat import time on Python < 3.10 withoutfrom __future__ import annotations. Since the Python version concern is project-wide, please also address it here if the project requires supporting Python < 3.10.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/environment.py` around lines 63 - 75, The __init__ signature in Environment uses Python 3.10 union syntax (e.g., list[float] | np.ndarray | None and WorkloadGenerator | None) which will raise on older Python versions; either add "from __future__ import annotations" at the top of src/environment.py to defer evaluation of annotations, or replace those unions with typing.Union/Optional (e.g., Optional[Union[list[float], np.ndarray]]) and typing.Optional[WorkloadGenerator] to be compatible with Python <3.10; update the annotations in the __init__ definition (and any other annotations in this file) accordingly to match the project's supported Python version.
74-74: Previously flaggedworkload_gentype mismatch is now resolved.The parameter is correctly typed as
WorkloadGenerator | None, matching the actual callers and thegenerate_jobssignature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/environment.py` at line 74, The workload_gen parameter typing has been corrected to WorkloadGenerator | None; keep this updated annotation on the function/class where workload_gen is declared, ensure the generate_jobs signature uses the same WorkloadGenerator | None type, remove any stale/duplicate type comments or alternate annotations that previously flagged a mismatch, and run mypy/unit tests to verify all callers (references to workload_gen and generate_jobs) type-check correctly.
🧹 Nitpick comments (3)
src/environment.py (1)
268-268: Unparameterizeddictinresetreturn type.The return type is annotated as
tuple[dict[str, np.ndarray], dict]. The second element — returned as{}— should bedict[str, Any]for consistency withstep'sdict[str, Any]and for stricter type checking.🧹 Proposed fix
- def reset(self, seed: int | None = None, options: dict[str, Any] | None = None) -> tuple[dict[str, np.ndarray], dict]: + def reset(self, seed: int | None = None, options: dict[str, Any] | None = None) -> tuple[dict[str, np.ndarray], dict[str, Any]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/environment.py` at line 268, Change the unparameterized dict in the reset return type to match step's metadata typing: update the signature of reset (function reset) from returning tuple[dict[str, np.ndarray], dict] to tuple[dict[str, np.ndarray], dict[str, Any]] so the second element (currently returned as {}) is properly typed for stricter type checking and consistency with step's dict[str, Any].src/prices.py (2)
34-35: Redundantself.external_prices = Noneassignment.
self.external_pricesis already initialized toNoneon Line 14. The reassignment on Line 35 inside theelsebranch is dead code.🧹 Proposed fix
else: - self.external_prices = None # Keep your defaults for normalization bounds self.MAX_PRICE = 24.0 self.MIN_PRICE = 16.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prices.py` around lines 34 - 35, The else-branch contains a redundant reassignment of the instance attribute self.external_prices to None (it was already initialized as None earlier); remove the redundant assignment in that else block so the attribute set at construction (self.external_prices) remains the single source of truth and avoid dead code in the method that contains the else branch.
97-101:advance_and_get_predicted_pricesshifts the window using the next external price, not the PREDICTION_WINDOW-ahead price.
get_next_price()fetchesexternal_prices[price_index], which is the element immediately after the initial window. After the first call it advancesprice_indexby 1. This means the new tail appended topredicted_pricesis the price just beyond the current window, which is the correct sliding-window behaviour. However, the docstring comment on Line 97 ("Changed name for readability") is a stale note from development — it adds no informational value to readers and should be cleaned up.🧹 Proposed fix
- def advance_and_get_predicted_prices(self) -> np.ndarray: # Changed name for readability + def advance_and_get_predicted_prices(self) -> np.ndarray:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/prices.py` around lines 97 - 101, The inline comment "Changed name for readability" in the advance_and_get_predicted_prices method is stale and should be removed; update the function's docstring (or add one) to concisely state that advance_and_get_predicted_prices calls get_next_price() to pull external_prices[price_index], advances price_index, shifts the predicted_prices window (using np.roll) and appends that next external price as the new tail; reference the symbols advance_and_get_predicted_prices, get_next_price, predicted_prices, price_index, and external_prices when editing so the behavioral note replaces the stale development comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/environment.py`:
- Line 67: The type mismatch for the parameter external_prices has been
corrected to list[float] | np.ndarray | None to match the Prices constructor
signature; no further code changes are necessary—verify the signature in the
Prices class and ensure any callers passing external_prices conform to the
updated type.
- Around line 63-75: The __init__ signature in Environment uses Python 3.10
union syntax (e.g., list[float] | np.ndarray | None and WorkloadGenerator |
None) which will raise on older Python versions; either add "from __future__
import annotations" at the top of src/environment.py to defer evaluation of
annotations, or replace those unions with typing.Union/Optional (e.g.,
Optional[Union[list[float], np.ndarray]]) and typing.Optional[WorkloadGenerator]
to be compatible with Python <3.10; update the annotations in the __init__
definition (and any other annotations in this file) accordingly to match the
project's supported Python version.
- Line 74: The workload_gen parameter typing has been corrected to
WorkloadGenerator | None; keep this updated annotation on the function/class
where workload_gen is declared, ensure the generate_jobs signature uses the same
WorkloadGenerator | None type, remove any stale/duplicate type comments or
alternate annotations that previously flagged a mismatch, and run mypy/unit
tests to verify all callers (references to workload_gen and generate_jobs)
type-check correctly.
In `@src/prices.py`:
- Around line 71-77: The synthetic branch now correctly applies start_index by
setting start = int(start_index), updating self.price_index, and generating
self.predicted_prices via self._synthetic_price_at over self.PREDICTION_WINDOW;
no code changes required—just remove the duplicate/obsolete review comment and
mark the change approved so the PR can be merged (references: start,
self.price_index, self.predicted_prices, _synthetic_price_at,
PREDICTION_WINDOW).
---
Nitpick comments:
In `@src/environment.py`:
- Line 268: Change the unparameterized dict in the reset return type to match
step's metadata typing: update the signature of reset (function reset) from
returning tuple[dict[str, np.ndarray], dict] to tuple[dict[str, np.ndarray],
dict[str, Any]] so the second element (currently returned as {}) is properly
typed for stricter type checking and consistency with step's dict[str, Any].
In `@src/prices.py`:
- Around line 34-35: The else-branch contains a redundant reassignment of the
instance attribute self.external_prices to None (it was already initialized as
None earlier); remove the redundant assignment in that else block so the
attribute set at construction (self.external_prices) remains the single source
of truth and avoid dead code in the method that contains the else branch.
- Around line 97-101: The inline comment "Changed name for readability" in the
advance_and_get_predicted_prices method is stale and should be removed; update
the function's docstring (or add one) to concisely state that
advance_and_get_predicted_prices calls get_next_price() to pull
external_prices[price_index], advances price_index, shifts the predicted_prices
window (using np.roll) and appends that next external price as the new tail;
reference the symbols advance_and_get_predicted_prices, get_next_price,
predicted_prices, price_index, and external_prices when editing so the
behavioral note replaces the stale development comment.
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.
No description provided.