-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature/rest api #12
Feature/rest api #12
Conversation
changed dependency version for autopep8 in pipfile which was causing an issue for pycodestyle dep conflict UIC-661
…inst pipenv usage UIC-661
Though the Size of docker build is now increased to 4.6GB from 2.6GB UIC-661
Falling back to pip due to increase in docker image size UIC-661
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 have gone through the code. Some smaller suggestion to incorporate.
I will run this today and validate this. It could be good if you can add soem sample (tested) api to try with.
@jollyparamjit can you validate pipenv based installation
|
||
__tablename__ = 'pandas_datasources' | ||
type = 'pandas' | ||
baselink = 'pandasdatasourcemodelview' # url portion pointing to ModelView |
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.
can we make URL_FOR related changes. I think that should be non impacting as that is a valid way of getting of url as well.
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.
aren't we doing it later after the other PR gets merged?
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.
our choice. but whatever we decide we need to ensure it is captured. Put a note in other PR/Task for this if we are defeering
# Local file, so just use Pandas directly | ||
data = self.source_url | ||
# Read the dataframe from the response | ||
self.df = self.pandas_read_method(data, **self.pandas_read_parameters) |
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 adding a blank line before this make it more readable as this is not part of else. However I am not sure about lint rules here
However I do see blank line used to make code readable and communicate thsi line is in parent block
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.
Not taking care these things in current PR, may be a separate US we can check the same.
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.
ok. I was more interested the knowing. We can follow where it make sense
contrib/connectors/pandas/models.py
Outdated
# col_obj is None, probably because the col is a metric, | ||
# in which case it is numeric anyway | ||
pass | ||
# query += '(\"{col}\" {op} \"{eq}\")'.format(col=col, op=op, eq=eq) |
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.
any reason to keep commented code
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.
not required, removed it
resp = super(PandasDatasourceModelView, self).edit(pk) | ||
if isinstance(resp, basestring): | ||
return resp | ||
return redirect('/superset/explore/pandas/{}/'.format(pk)) |
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.
here also we need to use url_for
if 'pandas' in ConnectorRegistry.sources: | ||
model = ConnectorRegistry.sources['pandas'].column_class | ||
modelview_to_model[model_view] = model | ||
|
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.
What is use of this. do we need to add ES here (current impl)?
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.
as in the above code, the new key for pandas was not working so had to stick with existing code, will see it later if we can find some good alternative
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.
add a commnet in code TODO:
To try using REST API interfacing, use the following API: The results can be displayed in table format |
* feat(rest-api): added rest api interface to superset WIP UIC-661 * feat(rest-api): upgraded rest api interface with python version 3.6.5 UIC-661 * feat(rest-api): refactored tox to work with pipenv changed dependency version for autopep8 in pipfile which was causing an issue for pycodestyle dep conflict UIC-661 * feat(rest-api): deleted requirements.txt and requirements-dev.txt against pipenv usage UIC-661 * feat(rest-api): added .python-version file UIC-661 * feat(pipenv-support): Docker build with pipenv Though the Size of docker build is now increased to 4.6GB from 2.6GB UIC-661 * feat(pipenv-support): Reverted Docker build with pipenv Falling back to pip due to increase in docker image size UIC-661 * feat(rest-api): removed unnecessary commented code UIC-661
* feat(rest-api): added rest api interface to superset WIP UIC-661 * feat(rest-api): upgraded rest api interface with python version 3.6.5 UIC-661 * feat(rest-api): refactored tox to work with pipenv changed dependency version for autopep8 in pipfile which was causing an issue for pycodestyle dep conflict UIC-661 * feat(rest-api): deleted requirements.txt and requirements-dev.txt against pipenv usage UIC-661 * feat(rest-api): added .python-version file UIC-661 * feat(pipenv-support): Docker build with pipenv Though the Size of docker build is now increased to 4.6GB from 2.6GB UIC-661 * feat(pipenv-support): Reverted Docker build with pipenv Falling back to pip due to increase in docker image size UIC-661 * feat(rest-api): removed unnecessary commented code UIC-661
* feat(rest-api): added rest api interface to superset WIP UIC-661 * feat(rest-api): upgraded rest api interface with python version 3.6.5 UIC-661 * feat(rest-api): refactored tox to work with pipenv changed dependency version for autopep8 in pipfile which was causing an issue for pycodestyle dep conflict UIC-661 * feat(rest-api): deleted requirements.txt and requirements-dev.txt against pipenv usage UIC-661 * feat(rest-api): added .python-version file UIC-661 * feat(pipenv-support): Docker build with pipenv Though the Size of docker build is now increased to 4.6GB from 2.6GB UIC-661 * feat(pipenv-support): Reverted Docker build with pipenv Falling back to pip due to increase in docker image size UIC-661 * feat(rest-api): removed unnecessary commented code UIC-661
REST API Support and pipenv support
Superset version
0.27.0
Description: