Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Uses /env to run python file #444

Merged
merged 9 commits into from
Nov 13, 2023
Merged

Uses /env to run python file #444

merged 9 commits into from
Nov 13, 2023

Conversation

ribalba
Copy link
Member

@ribalba ribalba commented Aug 31, 2023

Closes #438

@ribalba ribalba requested a review from ArneTR August 31, 2023 09:33
@github-actions
Copy link

github-actions bot commented Aug 31, 2023

Old Energy EstimationEco-CI Output: |Label|🖥 avg. CPU utilization [%]|🔋 Total Energy [Joules]|🔌 avg. Power [Watts]|Duration [Seconds]| |---|---|---|---|---| |Total Run|16.0564|1803.74|2.59905|702| |Measurement #1|16.1049|1803.74|2.59905|696|

📈 Energy graph:

 
 7.77 ┤                                                                                                                              ╭─╮
 7.17 ┤                                         ╭╮                                                 ╭╮╭╮                           ╭╮ │ │
 6.56 ┤                                         ││                ╭╮                               │╰╯│                 ╭╮        │╰╮│ │
 5.96 ┤            ╭─╮                          ││             ╭╮ ││                               │  │                ╭╯╰╮       │ ││ │
 5.35 ┤            │ │                          │╰╮           ╭╯│ ││                               │  │                │  ╰─╮    ╭╯ ╰╯ │
 4.75 ┤            │ │                          │ │           │ ╰╮│╰╮     ╭╮      ╭╮  ╭╮ ╭╮       ╭╯  ╰╮               │    │    │     │        ╭╮    ╭╮
 4.15 ┤            │ │╭╮╭─╮╭─────────╮╭─────────╯ ╰───────────╯  ╰╯ ╰───╮╭╯╰────╮ │╰──╯│ ││       │    ╰──────────╮    │    ╰───╮│     │        ││    ││                         ╭╮                                                         ╭╮                                                                                                       ╭╮                                                 ╭╮                                                                                ╭╮                                                                                                                                                                                                                               ╭
 3.54 ┤       ╭╮   │ ╰╯╰╯ ││         ╰╯                                 ││      │╭╯    │ ││    ╭╮ │               │   ╭╯        ││     │        ││ ╭─╮││          ╭╮          ╭╮ ││          ╭╮                                 ╭╮          ││          ╭╮                     ╭╮          ╭╮                                ╭╮                      │╰╮                   ╭╮ ╭╮╭╮                   ╭╮ │╰╮                   ╭╮ ╭╮╭╮                         ╭─╮          ╭╮            ╭╯╰╮                        ╭──╮          ╭╮                                    ╭╮                                ╭╮           ╭╮              ╭╮                        ╭╮          ╭╮            ╭─╮╭╮          ╭╮             ╭╯
 2.94 ┤      ╭╯│╭──╯      ╰╯                                            ╰╯      ││     ╰─╯╰╮╭╮ │╰─╯               ╰──╮│         ╰╯     │       ╭╯│╭╯ ││╰╮       ╭╮││╭╮       ╭╯│ │╰╮         │╰╮        ╭──╮        ╭──╮        │╰╮         │╰╮         │╰╮        ╭─╮         │╰╮        ╭╯╰╮        ╭─╮        ╭─╮         │╰╮        ╭──╮        ╭╯ │     ╭╮  ╭╮        ││ │╰╯╰╮        ╭╮       ╭╯│╭╯ ╰╮         ╭╮       │╰╮│╰╯╰╮       ╭──╮╭╮       ╭─╮╭╯ ╰╮       ╭─╯│╭╮       ╭─╮│  │        ╭────╮       ╭─╮│  ╰╮       ╭─╯│╭╮       ╭─╮╭──╮        ╭─╮        ╭╯╰╮         ╭╮        ╭─╮        ╭╯╰╮       ╭╮╭╯│╭╮╭╮          │╰╮         ╭──╮          ││        ╭─╯│╭╮       ╭╮╭╯ ╰╯│        ╭─╯│╭╮       ╭─╮╭╯
 2.34 ┤    ╭─╯ ╰╯                                                               ╰╯         ╰╯╰─╯                     ╰╯                ╰╮      │ ╰╯  ╰╯ │ ╭╮    │╰╯╰╯│       │ │╭╯ ╰╮     ╭╮ │ │   ╭╮  ╭╯  │        │  ╰╮       │ ╰╮    ╭╮  │ │        ╭╯ │        │ │         │ │╭╮      │  │        │ │        │ ╰╮        │ │        │  │        │  ╰╮    ││  ││       ╭╯│╭╯   │        ││       │ ││   │        ╭╯│       │ ││   │       │  ╰╯│       │ ││   │       │  ╰╯│       │ ││  ╰╮       │    │       │ ││   │       │  ╰╯│       │ ││  │        │ │        │  │        ╭╯│        │ │       ╭╯  │       │╰╯ ╰╯╰╯│  ╭╮   ╭─╮│ │       ╭─╯  │       ╭╮ │╰╮     ╭╮│  ╰╯│       │││    ╰╮       │  ╰╯│       │ ││
 1.73 ┼────╯                                                                                                                            ╰──────╯        ╰─╯╰────╯    ╰───────╯ ╰╯   ╰─────╯╰─╯ ╰───╯╰──╯   ╰────────╯   ╰───────╯  ╰────╯╰──╯ ╰────────╯  ╰────────╯ ╰─────────╯ ╰╯╰──────╯  ╰────────╯ ╰────────╯  ╰────────╯ ╰────────╯  ╰────────╯   ╰────╯╰──╯╰───────╯ ╰╯    ╰────────╯╰───────╯ ╰╯   ╰────────╯ ╰───────╯ ╰╯   ╰───────╯    ╰───────╯ ╰╯   ╰───────╯    ╰───────╯ ╰╯   ╰───────╯    ╰───────╯ ╰╯   ╰───────╯    ╰───────╯ ╰╯  ╰────────╯ ╰────────╯  ╰────────╯ ╰────────╯ ╰───────╯   ╰───────╯       ╰──╯╰───╯ ╰╯ ╰───────╯    ╰───────╯╰─╯ ╰─────╯╰╯    ╰───────╯╰╯     ╰───────╯    ╰───────╯ ╰╯
                                                                                                                                                                                                                                                                                                                                                          Watts over time
 ```</details>

@ArneTR
Copy link
Member

ArneTR commented Sep 1, 2023

LGTM. Two remarks:

  • Can you please move to escaped call in subprocess again. It should work like this: subprocess.run(['sudo', '/usr/bin/env', 'python3', python_file])
  • Please a comment for the good idea that the file should be non writeable by the user after making it sudo available

@ArneTR
Copy link
Member

ArneTR commented Sep 1, 2023

  • Please also add the functionality that the hardware_info with the root rights is not modified directly (as this would hinder a git pull), but rather a git-ignored copy is made that will be cleaned on install / update by the script with root rights

@ArneTR
Copy link
Member

ArneTR commented Sep 29, 2023

The so far made fixes should not be necessary now that we force a venv and Python is always running in venv mode.

The sys.executable and which pythton3 should be identical. However the locking down of the hardware_info is still needed. Can you schedule this in at some point in the next weeks?

@ribalba
Copy link
Member Author

ribalba commented Nov 6, 2023

This is still open as I need to move the hardware_info file to another file to make it owned by root

@ribalba
Copy link
Member Author

ribalba commented Nov 8, 2023

Ready to merge IMHO

@ArneTR ping

* main: (110 commits)
  Adding report url instead of just ID (#531)
  Makes error optional except if there is data we need (#530)
  Bump uvicorn[standard] from 0.23.2 to 0.24.0.post1 (#526)
  Adding current clocksource in linux according to timing problems outlined here: https://www.brendangregg.com/blog/2021-09-26/the-speed-of-time.html (#524)
  Bump orjson from 3.9.9 to 3.9.10 (#519)
  Bump pandas from 2.1.1 to 2.1.2 (#520)
  Update Dockerfile - Removed playwright version pinning (#523)
  Bump fastapi from 0.104.0 to 0.104.1 (#522)
  Casts everything to bigint in hog api
  Qol Updates (#521)
  build containers workflow: removed conditional from job scope and moved to step
  added options for build containers manual run to specify which container to build
  fix file permissions  for build-containers script
  Bump playwright/python in /docker/auxiliary-containers/gcb_playwright (#513)
  Bump pytest from 7.4.2 to 7.4.3 (#515)
  Added lshw to hardware_info_root (#514)
  Tag dockerhub containers (#518)
  Update XGBoost model
  Update README.md
  Bump pylint from 3.0.1 to 3.0.2 (#512)
  ...
@ArneTR
Copy link
Member

ArneTR commented Nov 10, 2023

The version commited works "by accident".

Actually the code suggests that actually the Python in the venv is used. That is not the case though.

WIth our new venv setup with correct include paths actually the /usr/bin/env should be removed and the actualy venv python binary location used.

To check that not the correct venv is used:

$ sudo /usr/bin/env python3 -c 'import sys; print(sys.executable)'
/usr/bin/python3

The expected line should be the same as when the venv is active:

$ /usr/bin/env python3 -c 'import sys; print(sys.executable)'
/home/gc/green-metrics-tool/venv/bin/python3
  1. I would argue for removing hardware_info_root.py from the tree now, as it shows up as modfied file afterwards.

  2. Main was not merged in before correctly. This leads to the fact that an addition to the hardware_info (lshw) got lost. Brought it back in.

  3. hardware_info_root_original.py was not venv ready.

I commited a fix to this. When using venvs though one could actually notice that it does not understand the /usr/bin/venv logic:

(venv) gc@palit:~/green-metrics-tool$ sudo /usr/bin/env python3 /home/gc/green-metrics-tool/lib/hardware_info_root.py
Traceback (most recent call last):
  File "/home/gc/green-metrics-tool/lib/hardware_info_root.py", line 10, in <module>
    from lib.hardware_info import rdr, rpwr, get_values
ModuleNotFoundError: No module named 'lib'

@ArneTR
Copy link
Member

ArneTR commented Nov 10, 2023

@ribalba Please review. IMHO ready to merge

@dan-mm This is in dire need of a test. We need to check that actually some data is returned that is root readable only. Does Github VM have the directory /sys/kernel/debug/sched ?

@ArneTR ArneTR removed their request for review November 10, 2023 13:00
Copy link

github-actions bot commented Nov 10, 2023

Old Energy EstimationEco-CI Output: |Label|🖥 avg. CPU utilization [%]|🔋 Total Energy [Joules]|🔌 avg. Power [Watts]|Duration [Seconds]| |---|---|---|---|---| |Total Run|9.10167|33336.7|49.6822|678| |Measurement #1|9.16753|33336.7|49.6822|672|

📈 Energy graph:

 
 122 ┤                                                            ╭╮                ╭╮
 114 ┤                                                            ││                ││
 106 ┤                                                            ││                ││
  98 ┤                                                           ╭╯│          ╭──╮  ││
  90 ┤                                                           │ │          │  ╰╮ │╰─╮
  83 ┤       ╭──╮    ╭╮                                          │ ╰─╮╭───╮   │   │ │  ╰╮           ╭╮
  75 ┤     ╭─╯  │    │╰╮          ╭╮            ╭╮        ╭╮     │   ╰╯   │   │   ╰─╯   │        ╭╮ ││                                                                                                                                                                                                                                    ╭╮                        ╭╮                        ╭╮                          ╭╮                         ╭╮                          ╭╮                                                                                      ╭╮                                                                                   ╭╮
  67 ┤     │    ╰╮  ╭╯ ╰────╮    ╭╯╰────────────╯╰────────╯╰╮    │        ╰╮ ╭╯         │        │╰─╯╰╮         ╭╮              ╭╮          ╭─╮         ╭─╮          ╭─╮          ╭─╮         ╭╮╭╮         ╭─╮         ╭─╮           ╭╮         ╭──╮         ╭─╮         ╭─╮         ╭─╮          ╭─╮         ╭──╮                        │╰╮                      ╭╯╰╮                   ╭╮ ╭╯╰╮         ╭╮            ╭─╯╰╮         ╭╮         ╭╮ ╭╯╰╮         ╭╮          ╭╮ ╭╯╰╮         ╭╮             ╭─╮          ╭╮         ╭──╮         ╭─╮          ╭╮         ││╭╮         ╭─╮            ╭╮╭╮                       ╭─╮                       ╭───╯╰╮         ╭╮         ╭╮ ╭─╮
  59 ┤    ╭╯     │  │       │    │                          ╰─╮╭╮│         │ │          │       ╭╯    │        ╭╯│╭╮        ╭╮╭─╯╰╮         │ │         │ ╰╮         │ │         ╭╯ │         │╰╯│         │ │         │ ╰╮        ╭─╯│         │  │         │ │         │ │         │ ╰╮        ╭╯ │         │  ╰╮                  ╭─╮╭─╯ │                   ╭╮╭╯  ╰╮        ╭╮        ││ │  │         ││╭╮        ╭╮│   │        ╭╯│         ││ │  ╰╮        ││╭╮        ││ │  │         ││╭╮        ╭╮╭╯ │         ╭╯│         │  │         │ │         ╭╯│         │╰╯│        ╭╯ │╭╮          │╰╯│         ╭─╮           │ ╰╮        ╭─╮           │     │        ╭╯│         ││ │ │        ╭
  51 ┤    │      │  │       │    │                            ╰╯╰╯         ╰╮│          │       │     ╰╮       │ │││       ╭╯││   │        ╭╯ │         │  │        ╭╯ ╰╮        │  │         │  │         │ │         │  │  ╭╮    │  │         │  │         │ │         │ ╰╮        │  │        │  ╰╮        │   │        ╭─╮     ╭╮│ ││   ╰╮        ╭╮       ╭╯││    │        ││       ╭╯│ │  ╰╮       ╭╯│││       ╭╯││   │        │ ╰╮        │╰╮│   │       ╭╯╰╯│        ││ │  ╰╮       ╭╯│││       ╭╯││  ╰╮        │ ╰╮        │  │        ╭╯ │         │ │         │  │        │  ╰╯│        ╭╮│  │        ╭╯ ╰╮          │  │       ╭╯ ╰╮       ╭╮ │     ╰╮       │ ╰╮        ││ │ │        │
  43 ┼────╯      ╰──╯       ╰────╯                                          ╰╯          ╰───────╯      ╰───────╯ ╰╯╰───────╯ ╰╯   ╰────────╯  ╰─────────╯  ╰────────╯   ╰────────╯  ╰─────────╯  ╰─────────╯ ╰─────────╯  ╰──╯╰────╯  ╰─────────╯  ╰─────────╯ ╰─────────╯  ╰────────╯  ╰────────╯   ╰────────╯   ╰────────╯ ╰─────╯╰╯ ╰╯    ╰────────╯╰───────╯ ╰╯    ╰────────╯╰───────╯ ╰─╯   ╰───────╯ ╰╯╰───────╯ ╰╯   ╰────────╯  ╰────────╯ ╰╯   ╰───────╯   ╰────────╯╰─╯   ╰───────╯ ╰╯╰───────╯ ╰╯   ╰────────╯  ╰────────╯  ╰────────╯  ╰─────────╯ ╰─────────╯  ╰────────╯    ╰────────╯╰╯  ╰────────╯   ╰──────────╯  ╰───────╯   ╰───────╯╰─╯      ╰───────╯  ╰────────╯╰─╯ ╰────────╯
                                                                                                                                                                                                                                                                                                                                              Watts over time
 ```</details>

Copy link

Eco-CI Output:

Label 🖥 avg. CPU utilization [%] 🔋 Total Energy [Joules] 🔌 avg. Power [Watts] Duration [Seconds]
Total Run 10.4134 36214.7 50.7208 721
Measurement #1 10.4814 36214.7 50.7208 715

📈 Energy graph:

 
 123 ┤                                                                                            ╭╮
 115 ┤                                                                                            ││                        ╭╮
 107 ┤                                                                                            │╰╮                  ╭╮  ╭╯│╭╮
  99 ┤                                                                                            │ │                  ││  │ │││
  91 ┤        ╭╮ ╭╮                                                                 ╭╮            │ │                ╭─╯╰╮ │ │││
  83 ┤        │╰─╯│                                 ╭╮                              ││            │ │ ╭────╮         │   ╰╮│ ╰╯│            ╭╮
  75 ┤       ╭╯   ╰╮      ╭─╮                   ╭╮  ││      ╭╮                     ╭╯│           ╭╯ ╰─╯    ╰╮        │    ││   │        ╭╮  ││                                                                                                                                           ╭╮                                                              ╭╮                                                   ╭╮                        ╭╮                          ╭╮                          ╭╮                          ╭╮                                                                                                                                                                          ╭╮
  67 ┤     ╭╮│     ╰──────╯ ╰───────────────────╯╰──╯╰──╮╭╮╭╯╰─────────────────────╯ ╰───────────╯          │        │    ╰╯   │        │╰──╯╰╮         ╭╮              ╭─╮         ╭─╮          ╭─╮         ╭──╮         ╭╮╭╮         ╭─╮         ╭──╮         ╭─╮           ╭╮         ││╭╮         ╭─╮          ╭╮          ╭─╮         ╭╮╭╮         ╭╯╰╮                   ╭╮ ╭───╮                      ╭╯╰╮                      ╭╯╰╮         ╭╮             ╭╯╰╮         ╭╮             ╭╯╰╮        ╭╮          ╭╮ ╭─╯╰╮         ╭╮          ╭╮ ╭─╮          ╭╮          ╭─╮          ╭╮          ╭╮          ╭─╮         ╭──╮            ╭─╮                       ╭─╮         ╭╮            ╭──╯╰╮                     ╭╮ ╭╮
  59 ┤    ╭╯╰╯                                          ││││                                                │        │         ╰╮       │     │        ╭╯│╭╮       ╭─╮╭─╯ │         │ ╰╮        ╭╯ │         │  │         │╰╯│         │ ╰╮        │  │         │ │         ╭─╯│         │╰╯│         │ │         ╭╯╰╮         │ │         │╰╯│         │  │         ╭╮        ││ │   │         ╭╮        ╭╮╭╯  │                   ╭╮ │  │         ││╭╮        ╭╮╭╯  │        ╭╯│         ╭─╮╭╯  │        │╰─╮        ││ │   │         ││╭╮        ││ │ ╰╮         │╰╮        ╭╯ │         ╭╯│         ╭╯│         ╭╯ ╰╮        │  │╭╮          │ ╰╮        ╭╮            │ │        ╭╯│           ╭╯    ╰╮        ╭─╮         ││ │╰╮        ╭
  51 ┤    │                                             ╰╯││                                                ╰╮       │          │      ╭╯     ╰╮       │ │││       │ ││   │         │  │        │  ╰╮        │  │         │  │         │  │        │  │         │ ╰╮        │  │         │  │         │ ╰╮        │  │   ╭╮   ╭╯ │         │  │        ╭╯  ╰╮        ││        ││ │   ╰╮        ││       ╭╯││   ╰╮        ╭╮       ╭╯│╭╯  ╰╮       ╭╯│││       ╭╯││   ╰╮       │ │╭╮       │ ││   │        │  │        ││ │   │       ╭╮││││       ╭╯│╭╯  │         │ │        │  ╰╮        │ ╰╮        │ ╰╮        │   │       ╭╯  │││       ╭╮ │  │       ╭╯╰─╮          │ ╰╮       │ │╭╮       ╭╮│      │       ╭╯ ╰╮       ╭╯│╭╯ ╰╮  ╭╮   │
  43 ┼────╯                                               ╰╯                                                 ╰───────╯          ╰──────╯       ╰───────╯ ╰╯╰───────╯ ╰╯   ╰─────────╯  ╰────────╯   ╰────────╯  ╰─────────╯  ╰─────────╯  ╰────────╯  ╰─────────╯  ╰────────╯  ╰─────────╯  ╰─────────╯  ╰────────╯  ╰───╯╰───╯  ╰─────────╯  ╰────────╯    ╰────────╯╰────────╯╰─╯    ╰────────╯╰───────╯ ╰╯    ╰────────╯╰───────╯ ╰╯    ╰───────╯ ╰╯╰───────╯ ╰╯    ╰───────╯ ╰╯╰───────╯ ╰╯   ╰────────╯  ╰────────╯╰─╯   ╰───────╯╰╯╰╯╰───────╯ ╰╯   ╰─────────╯ ╰────────╯   ╰────────╯  ╰────────╯  ╰────────╯   ╰───────╯   ╰╯╰───────╯╰─╯  ╰───────╯   ╰──────────╯  ╰───────╯ ╰╯╰───────╯╰╯      ╰───────╯   ╰───────╯ ╰╯   ╰──╯╰───╯
                                                                                                                                                                                                                                                                                                                                                                   Watts over time

@ribalba ribalba merged commit 55085a9 into main Nov 13, 2023
4 checks passed
@ribalba ribalba deleted the fix-sudoers-venv-bug branch November 13, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix sudo issue when using venv
2 participants