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

Update docs, remove unused option, small cleanups #50

Merged
merged 3 commits into from
Jan 8, 2024
Merged

Conversation

nweires
Copy link
Collaborator

@nweires nweires commented Jan 4, 2024

  • Update outdated docs and comments
  • Add an overview of the GCP architecture
  • Remove some unused code
  • Fix a couple small formatting issues

Tested: Did a small test run to check the code changes.

Copy link

github-actions bot commented Jan 4, 2024

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 f958eb7

code later. Also, because that branch has not yet been merged, this will also _not_ do any
refactoring right now to share code with that (to reduce merging complexity later). Instead, code
that's likely to be refactored out will be commented with 'todo: aws-shared'.
Architecture overview:
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 what the right level of detail here is, but I felt we were missing a high-level outline of how we're using GCP (the kind that would have been useful when we were first trying to understand the AWS implementation).

Copy link
Member

Choose a reason for hiding this comment

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

👍! I think you should add that many of these things are done by the base classes (including sampling, and input files).

I might suggest that a README.md would be better, especially since I think it'd be best to document more holistically (i.e., relationship with docker_base and aws), but I'm fine with this incremental improvement, at least.

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 a note that this logic is split between the two places.

Yeah, a readme might be better if we want to add more detail later. I'm thinking of this section as mainly "Here's how we're using GCP", which is pretty specific to gcp.py, but a more general "Here's how the code is structured" could also be helpful.

@nweires nweires requested a review from lathanh January 4, 2024 21:53
code later. Also, because that branch has not yet been merged, this will also _not_ do any
refactoring right now to share code with that (to reduce merging complexity later). Instead, code
that's likely to be refactored out will be commented with 'todo: aws-shared'.
Architecture overview:
Copy link
Member

Choose a reason for hiding this comment

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

👍! I think you should add that many of these things are done by the base classes (including sampling, and input files).

I might suggest that a README.md would be better, especially since I think it'd be best to document more holistically (i.e., relationship with docker_base and aws), but I'm fine with this incremental improvement, at least.

@nweires nweires merged commit 094b678 into gcp Jan 8, 2024
6 checks passed
@nweires nweires deleted the natalie/docs branch January 8, 2024 16:23
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