Refactor/refactor enrichment functions and use token #1034
Conversation
simon-contreras-deel
changed the title
Use do token in enrichment
|
Please @alejandrohall can you do a CR? |
| data_copy = data_copy.merge(data_geometry_id_enriched, on=_ENRICHMENT_ID, how='left')\ | ||
| .drop(_ENRICHMENT_ID, axis=1) | ||
| data_enriched = enrich(_prepare_sql, data=data, variables=variables, agg_operators=agg_operators, | ||
| data_geom_column=data_geom_column, filters=filters, credentials=credentials) |
There was a problem hiding this comment.
We can avoid requesting this parameter data_geom_column='geom' using https://github.com/CartoDB/cartoframes/blob/develop/cartoframes/utils/columns.py#L155. We would need to extract that method first.
Probably it is not a priority
There was a problem hiding this comment.
What about if the user has two columns with geometries? and what about if the column with geometries has a name not related to usual geometry names?
There was a problem hiding this comment.
I vote for not supporting dataframes with more than one geometry column by design, otherwise we'll end up trying to do magic to support corner cases.
About geometry column name, it has to be abstracted somehow (the columns module could work) otherwise we'll end up with different logic in different places to do the same thing.
My two cents :)
There was a problem hiding this comment.
vote for not supporting dataframes with more than one geometry column by design
+1 We are doing that way in DB too.
we'll end up with different logic in different places to do the same thing
@alejandrohall and I have talked about other things here, mainly describing the initial source of pain is not having geoDataFrames, but this is a bigger task.
So, I agree with @alrocar, that is the point. We should solve it in a similar fashion in every place. If you want @alejandrohall, I can do it
There was a problem hiding this comment.
Buy maybe we can do it in a different PR
There was a problem hiding this comment.
Okey lets remove the geo column name provided by the user!!
There was a problem hiding this comment.
Moved to an external issue #1046
| except RefreshError: | ||
| self.client = self._init_client() | ||
| try: | ||
| return func(self, *args, **kwargs) |
There was a problem hiding this comment.
Finally what happened with retries?
There was a problem hiding this comment.
It should not happen 2 times :) or the error should be different
| try: | ||
| return func(self, *args, **kwargs) | ||
| except RefreshError: | ||
| raise CartoException('Something went wrong accessing data. ' |
There was a problem hiding this comment.
I would add more information regarding the error, that is due to the token or smth because if the user doesn't have internet or whatever and get that error will contact with support.
There was a problem hiding this comment.
We are catching only RefreshError, so everything is working on the user side. But for some reason, the user is not able to refresh the token, but we don't want to share big query information to the user. This is why I have set this basic message about it.
| if isinstance(data, Dataset): | ||
| data = data.dataframe | ||
|
|
||
| data_copy = data.copy() |
There was a problem hiding this comment.
Should we make this copy? #1045
simon-contreras-deel
deleted the
refactor/refactor_enrichment_functions
branch
Solves a part of https://github.com/CartoDB/data-observatory/issues/147