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

Convert all plugin rezconfig to rezconfig.py #1692

Conversation

brycegbrazen
Copy link
Contributor

@brycegbrazen brycegbrazen commented Mar 19, 2024

Effectively copied over work that @nerdvegas did back on this commit.

Closes #525.

Signed-off-by: brycegbrazen <bryce.gattis@brazenanimation.com>
@brycegbrazen brycegbrazen requested a review from a team as a code owner March 19, 2024 17:38
Signed-off-by: brycegbrazen <bryce.gattis@brazenanimation.com>
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.27%. Comparing base (3c75a19) to head (00a18a8).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1692      +/-   ##
==========================================
+ Coverage   58.25%   58.27%   +0.01%     
==========================================
  Files         126      126              
  Lines       17157    17157              
  Branches     3504     3504              
==========================================
+ Hits         9995     9998       +3     
+ Misses       6496     6494       -2     
+ Partials      666      665       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: brycegbrazen <bryce.gattis@brazenanimation.com>
Signed-off-by: brycegbrazen <bryce.gattis@brazenanimation.com>
Signed-off-by: brycegbrazen <bryce.gattis@brazenanimation.com>
Signed-off-by: brycegbrazen <bryce.gattis@brazenanimation.com>
@JeanChristopheMorinPerso
Copy link
Member

JeanChristopheMorinPerso commented Mar 23, 2024

Thanks @brycegbrazen. Did you notice a performance different between loading configs from python files instead of YAML? I feel like it'll be slightly faster with python files than with YAML since YAML is hard to parse and hence slow. But we should check to make sure there is no unexpected regression.

We could use https://github.com/sharkdp/hyperfine to test rez config plugins's speed on this PR versus the code we have on main.

@BryceGattis
Copy link
Contributor

@JeanChristopheMorinPerso I didn't benchmark anything at this point, but that does sound like a great idea.

Should rez-benchmark include the ability to run CLI benchmarks like this? Or perhaps we should just set-up a new GitHub CI test that runs like all of our other ones?

@brycegbrazen
Copy link
Contributor Author

I ran hyperfine before my update (on main):

hyperfine --warmup 3 "rez config plugins"
Benchmark 1: rez config plugins
  Time (mean ± σ):     427.9 ms ±   9.4 ms    [User: 204.7 ms, System: 180.3 ms]
  Range (min … max):   417.7 ms … 449.8 ms    10 runs

And then after the update:

hyperfine --warmup 3 "rez config plugins"
Benchmark 1: rez config plugins
  Time (mean ± σ):     429.7 ms ±  14.6 ms    [User: 217.5 ms, System: 179.1 ms]
  Range (min … max):   412.7 ms … 460.1 ms    10 runs

@JeanChristopheMorinPerso JeanChristopheMorinPerso added this to the Next milestone Apr 7, 2024
@JeanChristopheMorinPerso
Copy link
Member

Thanks. I also tested on my machine and I see approximately the same stats for both:

Benchmark 1: /tmp/asd/old/bin/rez/rez-config plugins
  Time (mean ± σ):     250.1 ms ±   4.2 ms    [User: 206.3 ms, System: 42.7 ms]
  Range (min … max):   244.1 ms … 260.5 ms    11 runs
 
Benchmark 2: /tmp/asd/new/bin/rez/rez-config plugins
  Time (mean ± σ):     251.7 ms ±   4.5 ms    [User: 211.3 ms, System: 39.3 ms]
  Range (min … max):   246.0 ms … 262.8 ms    11 runs
 
Summary
  /tmp/asd/old/bin/rez/rez-config plugins ran
    1.01 ± 0.02 times faster than /tmp/asd/new/bin/rez/rez-config plugins

@JeanChristopheMorinPerso
Copy link
Member

(correction, I wrongly installed rez in the previous run, my bad)

Benchmark 1: /tmp/asd/old/bin/rez/rez-config plugins
  Time (mean ± σ):     265.6 ms ±   4.0 ms    [User: 227.4 ms, System: 36.7 ms]
  Range (min … max):   258.2 ms … 270.2 ms    11 runs
 
Benchmark 2: /tmp/asd/new/bin/rez/rez-config plugins
  Time (mean ± σ):     250.6 ms ±   4.9 ms    [User: 214.5 ms, System: 34.4 ms]
  Range (min … max):   243.8 ms … 258.3 ms    11 runs
 
Summary
  /tmp/asd/new/bin/rez/rez-config plugins ran
    1.06 ± 0.03 times faster than /tmp/asd/old/bin/rez/rez-config plugins

Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
@JeanChristopheMorinPerso JeanChristopheMorinPerso merged commit 1b9c72a into AcademySoftwareFoundation:main Apr 13, 2024
53 checks passed
Pixel-Minions added a commit to Pixel-Minions/rez that referenced this pull request Sep 26, 2024
…on#1692)

Signed-off-by: brycegbrazen <bryce.gattis@brazenanimation.com>
Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
Co-authored-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>
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.

cleanup rezconfigs in plugins
3 participants