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

Clean up output dir validation #67

Merged
merged 3 commits into from
Feb 15, 2024
Merged

Clean up output dir validation #67

merged 3 commits into from
Feb 15, 2024

Conversation

nweires
Copy link
Collaborator

@nweires nweires commented Feb 13, 2024

This moves the "check for existing files in the output dir" step out of the validation function.

There are enough cases where we do validation but don't want to do this check that I think it makes sense to keep it separate. (--missingonly, --clean, --postprocessonly, --showjobs)

Copy link

File Coverage
All files 86%
base.py 91%
exc.py 57%
hpc.py 78%
local.py 70%
postprocessing.py 84%
utils.py 91%
cloud/docker_base.py 78%
sampler/base.py 79%
sampler/downselect.py 33%
sampler/precomputed.py 93%
sampler/residential_quota.py 61%
test/shared_testing_stuff.py 85%
test/test_docker.py 33%
test/test_local.py 97%
test/test_validation.py 97%
workflow_generator/base.py 90%
workflow_generator/commercial.py 53%
workflow_generator/residential_hpxml.py 86%

Minimum allowed coverage is 33%

Generated by 🐒 cobertura-action against da5546c

@@ -1226,7 +1235,7 @@ def main():
logger.setLevel(logging.INFO)

# validate the project, and if --validateonly flag is set, return True if validation passes
GcpBatch.validate_project(os.path.abspath(args.project_filename), args.missingonly)
GcpBatch.validate_project(os.path.abspath(args.project_filename))

Choose a reason for hiding this comment

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

should we also add args.missingonly here and avoid checking for that in the check_output_dir?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if this is what you meant, but I moved the missingonly check out of check_output_dir and put it below.

user_choice = (
input(
f"Output files are already present in bucket {self.gcs_bucket}! For example, {blob.name} exists. "
f"Do you want to delete all the files in {prefix_for_deletion}? (yes/no): "

Choose a reason for hiding this comment

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

Thinking maybe we add also the warning that if you delete these files, you won't be able to recover them or something like that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added "permanently" to the prompt. Do you think that's enough?

@nweires nweires merged commit 65c8703 into gcp Feb 15, 2024
0 of 4 checks passed
@nweires nweires deleted the natalie/validation branch February 15, 2024 22:16
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.

2 participants