epics-containers / edge-containers-cli

command line shortcuts for epics containers developers
Apache License 2.0
3 stars 1 forks source link

Resolve symlinks when checking EC service versions #128

Closed marcelldls closed 2 months ago

marcelldls commented 2 months ago

Fixes #127 - This PR introduces the following to ec instances:

marcelldls commented 2 months ago

@gilesknap I did a re-run of the last scuessful workflow run for this repo and it now fails. Does https://github.com/epics-containers/edge-containers-cli/actions/runs/8469131442/job/24112327403#step:7:541 make any sence to you? I believe this issue is making my tests fail (they pass if I run them locally)

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 94.23077% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 86.78%. Comparing base (7282c71) to head (4ed4639).

Files Patch % Lines
src/edge_containers_cli/git.py 91.89% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #128 +/- ## ========================================== - Coverage 86.92% 86.78% -0.15% ========================================== Files 14 14 Lines 788 802 +14 ========================================== + Hits 685 696 +11 - Misses 103 106 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gilesknap commented 2 months ago

@gilesknap I did a re-run of the last scuessful workflow run for this repo and it now fails. Does https://github.com/epics-containers/edge-containers-cli/actions/runs/8469131442/job/24112327403#step:7:541 make any sence to you? I believe this issue is making my tests fail (they pass if I run them locally)

Maybe - I got the tests to run anyway. I'm thinking that the globbing of files in the config folder has non-deterministic order.

gilesknap commented 2 months ago

This looks good. How much has it slowed down completion?

marcelldls commented 2 months ago

This looks good. How much has it slowed down completion?

As I said in the initial comment, this PR actually results in a performance improvement compared to what we have. The reason is because we were doing a git diff num service x num tags times. Now we only it num tags times. The symlinks dont seem to add much overhead.

ec instances Time to do 10
Current 19.8s
This PR 12.2s
This PR + Skip symlinks 10.8s

*I use time for run in {1..10}; do ec instances bl01c-di-dcam-01; done

marcelldls commented 2 months ago

Although looking at it now, I think I just want to consider the edge cases a bit more. Also maybe its also a bit long as a single function

gilesknap commented 2 months ago

While you are thinking about that. What do you think about having some completion cache in a file and keeping it for a little longer? I find auto completion a little too sluggish.

This will only get worse once we use these repos in anger and have many tags

marcelldls commented 2 months ago

While you are thinking about that. What do you think about having some completion cache in a file and keeping it for a little longer? I find auto completion a little too sluggish.

This will only get worse once we use these repos in anger and have many tags

We already do this: https://github.com/epics-containers/edge-containers-cli/blob/7282c71f20f679490a6f3a907f603994fe5f410b/src/edge_containers_cli/globals.py#L33
https://github.com/epics-containers/edge-containers-cli/blob/7282c71f20f679490a6f3a907f603994fe5f410b/src/edge_containers_cli/autocomplete.py#L38-L43 I initially thought this was too short, but it also means that you wont be waiting for changes.

I believe the git clone has the largest overhead. If I do time for run in {1..10}; do git clone https://gitlab.diamond.ac.uk/controls/containers/beamline/bl01c.git; rm -rf bl01c; done - It takes 7.4 seconds. However time for run in {1..10}; do git pull; done takes only 1.8 seconds. So maybe we should check if there is already a repo present in temp and if so do a git pull rather then make sure we have the latest?

marcelldls commented 2 months ago
Maybe an interesting comparison is actually to run time for run in {1..10}; do ec --version; done as a baseline Command Time to do 10
ec instances Current 19.8s
ec instances This PR 12.2s
ec instances This PR + Skip symlinks 10.8s
ec --version 5.6s

which I believe stands to show that chasing further improvements will give diminishing returns versus the limitations of the project/framework as it currently stands

marcelldls commented 2 months ago

@gilesknap I think im happy with this for now. Shall we merge?

gilesknap commented 2 months ago

Yeah sure. Re the timings: where is your venv? Local or remote filesystem?