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

Mapd session #1796

Merged
merged 9 commits into from May 29, 2019

Conversation

Projects
3 participants
@ian-r-rose
Copy link
Contributor

commented May 24, 2019

pymapd recently gained the ability to connect via a pre-authenticated session ID. It would be nice if the Ibis mapd connector could do that as well. cc @xmnlab

Show resolved Hide resolved ibis/mapd/tests/conftest.py Outdated
Show resolved Hide resolved ci/requirements-3.5-dev.yml Outdated
Show resolved Hide resolved ibis/mapd/api.py Outdated
Show resolved Hide resolved ibis/mapd/tests/test_client.py Outdated
Show resolved Hide resolved setup.py Outdated

@cpcloud cpcloud added this to the 1.1.0 milestone May 25, 2019

@cpcloud cpcloud added this to In progress in OmniSci via automation May 25, 2019

@cpcloud cpcloud added the omnisci label May 25, 2019

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

Thanks for the review @cpcloud!

cpcloud added a commit that referenced this pull request May 27, 2019

BUG/CI/BLD: Fix Python 3.5 dependency versions
@ian-r-rose Can you take a look at this and make sure that it matches
our discussion re OmniSci on #1796?
Author: Phillip Cloud <cpcloud@gmail.com>

Closes #1798 from cpcloud/fix-py35-deps and squashes the following commits:

2bace3f [Phillip Cloud] Add pydata-google-auth to pip deps in conda env requirements
4216d7a [Phillip Cloud] Fix Python 3.5 dependency versions
@cpcloud

This comment has been minimized.

Copy link
Member

commented May 27, 2019

@ian-r-rose Can you rebase to pick up the changes from #1798?

@ian-r-rose ian-r-rose force-pushed the ian-r-rose:mapd-session-id branch from 0d646ac to 403cbef May 27, 2019

@ian-r-rose ian-r-rose force-pushed the ian-r-rose:mapd-session-id branch from 2b2cafb to c60e105 May 27, 2019

@cpcloud

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@cpcloud cpcloud self-assigned this May 28, 2019

Show resolved Hide resolved ibis/mapd/tests/test_client.py Outdated

OmniSci automation moved this from In progress to Needs review May 28, 2019

@cpcloud
Copy link
Member

left a comment

Small nit, then this is good to go.

)
if session_id:
if self.version < pkg_resources.parse_version('0.12.0'):
raise Exception('Must have pympad > 0.12 to use session ID')

This comment has been minimized.

Copy link
@cpcloud

cpcloud May 29, 2019

Member

Can you add an Exception subclass for this? E.g., PyMapDVersionError. There's also a typo in pympad.

This comment has been minimized.

Copy link
@ian-r-rose

ian-r-rose May 29, 2019

Author Contributor

Sure thing.

@jp-harvey

This comment has been minimized.

Copy link

commented May 29, 2019

@cpcloud @ian-r-rose it would also be helpful if one could pass an existing pymapd connection object to Ibis instead of having to build a connection every time. Is this something we could / should include in this PR?

@cpcloud

This comment has been minimized.

Copy link
Member

commented May 29, 2019

LGTM. Merging! Thanks.

@cpcloud

This comment has been minimized.

Copy link
Member

commented May 29, 2019

@jp-harvey I think we'd probably want another API like ibis.mapd.from_pymapd(existing_connection). Can you open an issue to track this?

@ian-r-rose

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Awesome, thanks for the reviews @cpcloud! Do you have any thoughts about @jp-harvey's suggestion?

@cpcloud cpcloud merged commit f2df192 into ibis-project:master May 29, 2019

13 checks passed

ci/circleci: python35_test Your tests passed on CircleCI!
Details
ci/circleci: python36_benchmark Your tests passed on CircleCI!
Details
ci/circleci: python36_conda_build Your tests passed on CircleCI!
Details
ci/circleci: python36_docs Your tests passed on CircleCI!
Details
ci/circleci: python36_test Your tests passed on CircleCI!
Details
ci/circleci: python37_conda_build Your tests passed on CircleCI!
Details
ci/circleci: python37_test Your tests passed on CircleCI!
Details
ibis-project.ibis Build #20190529.2 succeeded
Details
ibis-project.ibis (WindowsCondaBuild py36) WindowsCondaBuild py36 succeeded
Details
ibis-project.ibis (WindowsCondaBuild py37) WindowsCondaBuild py37 succeeded
Details
ibis-project.ibis (WindowsTest py35) WindowsTest py35 succeeded
Details
ibis-project.ibis (WindowsTest py36) WindowsTest py36 succeeded
Details
ibis-project.ibis (WindowsTest py37) WindowsTest py37 succeeded
Details

OmniSci automation moved this from Needs review to Done May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.