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

Add option to set upload chunk size in project config #53

Merged
merged 7 commits into from
Jan 10, 2024

Conversation

nweires
Copy link
Collaborator

@nweires nweires commented Jan 9, 2024

  • Add the option to change upload chunk size.
  • Log a more useful error message telling users when to use this setting.

Testing:

  • I was able to reproduce the error by setting a large chunk size and running from my laptop. The resulting error is ConnectionError(ProtocolError('Connection aborted.', TimeoutError('The write operation timed out'))).

Copy link

github-actions bot commented Jan 9, 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 2a3fae5

)
except requests.exceptions.ConnectionError:
logger.error("Error while uploading files to GCS bucket.")
logger.error(
Copy link
Member

Choose a reason for hiding this comment

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

  1. I think it'd be better for this to be just one log message.

  2. Logging and rethrowing is somewhat of an anti-pattern, though I'm less sure about that for Python (but I think the principles still apply).

    • How that applies here, theoretically, is that the stack trace (which tells us whether this is a timeout error or something else) could be distant from this message about it. Admittedly, in this case there probably won't be any distance.

Given those, I think what you should do instead is raise a new exception (that contains this info) chained with the original exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point - fixed.

Copy link

@mfathollahzadeh mfathollahzadeh left a comment

Choose a reason for hiding this comment

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

Thanks! This looks great!

@nweires nweires merged commit 3657fb4 into gcp Jan 10, 2024
5 checks passed
@nweires nweires deleted the natalie/upload_settings branch January 10, 2024 20:00
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.

3 participants