-
Notifications
You must be signed in to change notification settings - Fork 63
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
Check account disk quotas before writing using to_carto #1674
Check account disk quotas before writing using to_carto #1674
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job. Some minor comments added. I miss also two units tests to cover the new feature (working and exception raised)
cartoframes/io/carto.py
Outdated
@@ -89,6 +91,9 @@ def to_carto(dataframe, table_name, credentials=None, if_exists='fail', geom_col | |||
uses the name of the index from the dataframe. | |||
cartodbfy (bool, optional): convert the table to CARTO format. Default True. More info | |||
`here <https://carto.com/developers/sql-api/guides/creating-tables/#create-tables>`. | |||
ignore_quota_warning (bool, optional): ignore the warning of the possible quota exceeded | |||
and force the upload. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add here Default False.
to be consistent with the other descriptions
cartoframes/io/carto.py
Outdated
@@ -114,6 +119,17 @@ def to_carto(dataframe, table_name, credentials=None, if_exists='fail', geom_col | |||
|
|||
context_manager = ContextManager(credentials) | |||
|
|||
if not ignore_quota_warning: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could extract here me_data = context_manager.credentials.me_data
to have a bit more readable code.
cartoframes/io/carto.py
Outdated
if not ignore_quota_warning: | ||
if context_manager.credentials.me_data is not None and context_manager.credentials.me_data.get('user_data'): | ||
n = min(SAMPLE_ROWS_NUMBER, len(dataframe)) | ||
dataframe_size = len(dataframe.sample(n=n).to_csv(header=False)) * len(dataframe) / n / CSV_TO_CARTO_RATIO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename this dataframe_size
to estimated_byte_size
cartoframes/io/carto.py
Outdated
@@ -89,6 +91,9 @@ def to_carto(dataframe, table_name, credentials=None, if_exists='fail', geom_col | |||
uses the name of the index from the dataframe. | |||
cartodbfy (bool, optional): convert the table to CARTO format. Default True. More info | |||
`here <https://carto.com/developers/sql-api/guides/creating-tables/#create-tables>`. | |||
ignore_quota_warning (bool, optional): ignore the warning of the possible quota exceeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use skip
instead of ignore
in the param name. What do you think?
cartoframes/io/carto.py
Outdated
n = min(SAMPLE_ROWS_NUMBER, len(dataframe)) | ||
estimated_byte_size = len(dataframe.sample(n=n).to_csv(header=False)) * len(dataframe) \ | ||
/ n / CSV_TO_CARTO_RATIO | ||
remaining_byte_quota = context_manager.credentials.me_data.get('user_data').get('remaining_byte_quota') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more me_data
here
@Jesus89 second CR round? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
For the acceptance, could you try an auto-generated CSV file to test the real limits?
Check account disk quotas before writing using
to_carto
Note: The PR is blocked until we decide what to do with in discussion here https://app.clubhouse.io/cartoteam/story/93719/check-account-disk-quotas-before-writing