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

Use cable local checkout #255

Merged
merged 7 commits into from
Mar 6, 2024
Merged

Use cable local checkout #255

merged 7 commits into from
Mar 6, 2024

Conversation

abhaasgoyal
Copy link

@abhaasgoyal abhaasgoyal commented Feb 28, 2024

Resolves #33

Merge Message

  • Add keyword to specify realisations in local paths
  • Enable usage of benchcab clean to remove directories created by benchcab, which cleanly removes symlinks. In case a user runs benchcab when src already exists, the user would be shown a suggestion to run benchcab clean.
  • benchcab clean further options - submissions deletes submission related files/dirs, realisations deletes realisation related files/dirs, all does both
  • Add integration test for local checkout of CABLE in addition to git repository (as of now they point to the same supported commit where CABLE successfully builds)

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

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

Project coverage is 63.84%. Comparing base (ce3608e) to head (ac2cb1f).
Report is 1 commits behind head on main.

Files Patch % Lines
benchcab/utils/repo.py 22.72% 17 Missing ⚠️
benchcab/benchcab.py 8.33% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
+ Coverage   63.50%   63.84%   +0.33%     
==========================================
  Files          35       35              
  Lines        2521     2597      +76     
==========================================
+ Hits         1601     1658      +57     
- Misses        920      939      +19     

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

@abhaasgoyal abhaasgoyal force-pushed the 33-use-cable-local-checkout branch 2 times, most recently from 6dafe47 to 3f2f731 Compare March 1, 2024 03:39
@abhaasgoyal abhaasgoyal self-assigned this Mar 1, 2024
@abhaasgoyal abhaasgoyal marked this pull request as ready for review March 4, 2024 00:48
@abhaasgoyal abhaasgoyal changed the title DRAFT: Use cable local checkout Use cable local checkout Mar 4, 2024
@abhaasgoyal abhaasgoyal force-pushed the 33-use-cable-local-checkout branch 3 times, most recently from c46accf to 4218b87 Compare March 4, 2024 03:19
Copy link
Collaborator

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks fine. But a few things to look into, especially in the tests.

Also, the documentation changes need to be added to this PR.

benchcab/data/test/integration.sh Show resolved Hide resolved
benchcab/data/test/integration.sh Outdated Show resolved Hide resolved
benchcab/utils/repo.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
benchcab/benchcab.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I only had small things to suggest but it turned into big things.

  1. We don't want to remove rev_number-*.log. These files were intended to allow to reproduce a previous run by recording the commit/revision numbers. The mechanism needs some review to be more useful, to be done later. But we shouldn't put in place anything that removes these files between realisations, otherwise they are completely useless. I have put in review suggestions which I think are enough to remove this but please check everything is gone.
  2. I think we should phase out internal.CWD. It is a remnant from the start of the code and it isn't really needed now that we are using context managers to change directories. So can you rewrite the clean functions to use Path.cwd() instead? And then the tests.

The rest of my comments are more minor stuff.

benchcab/utils/repo.py Show resolved Hide resolved
benchcab/cli.py Outdated Show resolved Hide resolved
docs/user_guide/config_options.md Outdated Show resolved Hide resolved
benchcab/cli.py Outdated Show resolved Hide resolved
benchcab/workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
tests/test_workdir.py Show resolved Hide resolved
tests/test_workdir.py Outdated Show resolved Hide resolved
abhaasgoyal added a commit that referenced this pull request Mar 6, 2024
abhaasgoyal added a commit that referenced this pull request Mar 6, 2024
@abhaasgoyal
Copy link
Author

abhaasgoyal commented Mar 6, 2024

rm benchmark_cable_qsub.sh* rev_number-*; rm -rf runs/ src/

Here (see surrounding lines as well), it is suggested to remove rev_number-* as well, I will change it to benchcab clean all here, however should we reatain rev_number-* according to the comment #255 (review)

Edit: the changes are done here in case if there's any change required

benchcab clean all

abhaasgoyal added a commit that referenced this pull request Mar 6, 2024
abhaasgoyal added a commit that referenced this pull request Mar 6, 2024
abhaasgoyal added a commit that referenced this pull request Mar 6, 2024
Copy link
Collaborator

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small changes. I approve as I think you can make these without me reviewing.

tests/test_workdir.py Outdated Show resolved Hide resolved
docs/user_guide/config_options.md Show resolved Hide resolved
@abhaasgoyal abhaasgoyal merged commit 3e702e8 into main Mar 6, 2024
5 checks passed
@abhaasgoyal abhaasgoyal deleted the 33-use-cable-local-checkout branch March 6, 2024 05:59
@SeanBryan51 SeanBryan51 mentioned this pull request Apr 2, 2024
2 tasks
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.

Use local checkout of CABLE
2 participants