Elephant #1520
Elephant #1520
Conversation
|
Skipped: This PR does not contain any of your configured keywords: ( |
* main: (34 commits) Updated Cloud Energy Writing 10x as OOM does not trigger in GitHub VM (fix): email report should use only run name and not full email simple title for subject (fix): run_id foreign key must be added after runs table create to resolve two way dependency (feat): Added machine to email reports (fix): SQL syntax error (feat): Removing actually unnecessary run_id update on the jobs table Also added migration (feat): Added run_id in Jobs table so we can reference the run and send proper email templates (fix): Switching on DB job_type and not on supplied one (fix): EmailXJob must expand request to all possible email jobs (feat): EmailXJob centralized to the email keyword to have only only cronjob to trigger Email templates (#1518) (fix): commit_hash adding as href will not trigger any XSS problems which might come from user input here (feat): Not asking for activation of Eco CI, PowerHog and CarbonDB anymore as the components are now open sourced and fully available Encoding errors (#1519) (fix): Ported Available Memory fix also to macOS Updated Cloud Energy Added workflow dispatch trigger Delete old packages worklflow added ...
[skip ci]
ArneTR
left a comment
ArneTR
left a comment
There was a problem hiding this comment.
Code integration makes a good impression although I am unsure about some namings and also how carbon is calculated and displayed.
Code related stuff in this Review. Design related stuff and testing via email
docker/structure.sql
Outdated
| filename text, | ||
| usage_scenario_variables jsonb NOT NULL DEFAULT '{}', | ||
| category_ids int[], | ||
| carbon-simulation jsonb NOT NULL DEFAULT '{}', |
There was a problem hiding this comment.
can this be an underscore to align with other column names?
There was a problem hiding this comment.
Furthermore: Is currently anywhere a job stored? I could not find it. My question here: The column is JSONB. How will it hold the UUID?
| "source": "Formula (XGBoost)", | ||
| "explanation": "Machine operational CO₂ calculated by formula via XGBoost estimation" | ||
| }, | ||
| "psu_carbon_elephant_machine": { |
There was a problem hiding this comment.
Ideas for more aligned name:
- grid_carbon_intensity_api_global
- grid_carbon_intensity_elephant_global
Instead of _global maybe also _zone or _region would be more fitting.
lib/job/base.py
Outdated
| query = """ | ||
| INSERT INTO | ||
| jobs (run_id, type, name, url, email, branch, filename, usage_scenario_variables, category_ids, machine_id, user_id, message, state, created_at) | ||
| jobs (type, name, url, email, branch, filename, usage_scenario_variables, carbon, machine_id, user_id, message, state, created_at) |
There was a problem hiding this comment.
why is the filed differently called than the parameter of the function? For me this creates additional cognitive load to understand if there is a differentiation
lib/job/base.py
Outdated
| j.id, j.run_id, j.type, j.state, j.name, j.email, j.url, j.branch, | ||
| j.filename, j.usage_scenario_variables, j.category_ids, j.machine_id, j.user_id, m.description, j.message, j.created_at | ||
| j.id, j.state, j.name, j.email, j.url, j.branch, | ||
| j.filename, j.usage_scenario_variables, j.machine_id, j.user_id, m.description, j.message, r.id as run_id, j.created_at, j.carbon_simulation |
There was a problem hiding this comment.
here it is now called carbon_simulation again? Bug?
| if not energy_values: | ||
| continue | ||
|
|
||
| detail_name = f"{energy_metric}_{energy_detail_name}_{carbon_metric}_{carbon_detail_name}" |
There was a problem hiding this comment.
that is very different than the current naming scheme where only energy is replaced with carbon ...
what is the benefit of creating a metric with elephant in the name here from the energy_machine metrics?
| f"Electricity Maps base URL {API_PAST_URL} could not be reached: {exc}") from exc | ||
|
|
||
| finally: | ||
| if response is not None: |
There was a problem hiding this comment.
maybe a with block rather?
| f"Elephant base URL {self._elephant_base_url} could not be reached: {exc}") from exc | ||
|
|
||
| def get_stderr(self): | ||
| return error_string |
There was a problem hiding this comment.
why a global? When are you accessing this outside the class context?
| if not energy_values: | ||
| continue | ||
|
|
||
| detail_name = f"{energy_metric}_{energy_detail_name}_{carbon_metric}_{carbon_detail_name}" |
There was a problem hiding this comment.
that is very different than the current naming scheme where only energy is replaced with carbon ...
what is the benefit of creating a metric with elephant in the name here from the energy_machine metrics?
| ) | ||
|
|
||
| def _configure_elephant(self): | ||
| if not self.location: |
| return error_string | ||
|
|
||
| def start_profiling(self, _=None): | ||
| self._start_time = datetime.now(timezone.utc) |
There was a problem hiding this comment.
If this is an internal working var and should never be seen otherwise maybe consider moving to self.__
|
I also fixed some bugs like renaming the db field in the jobs table from carbon to carbn_simulation ... normalzing the parameter to carbon_simulation in args, function signatures etc. |
No description provided.