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

ENH: avoid unnecessary ga mgmt calls if given profile_id upfront #4529

Closed
wants to merge 1 commit into from
Closed

ENH: avoid unnecessary ga mgmt calls if given profile_id upfront #4529

wants to merge 1 commit into from

Conversation

tshauck
Copy link
Contributor

@tshauck tshauck commented Aug 10, 2013

profile_id is ultimately what's needed to make a call to the reporting api. If it is specified in the parameter list it seems wasteful to call the management api 3 times.

@jreback
Copy link
Contributor

jreback commented Aug 10, 2013

  • can u hook up Travis (see contributing.md)? - also pls verify that Travis is actually testing the ga tests (eg make sure it's not skipping them)
  • there is an issue out there to add some (even very basic) docs for ga
    could u do that google analytics docs #3508?

@tshauck
Copy link
Contributor Author

tshauck commented Aug 10, 2013

  • It's building the Travis env now... I'll make sure it runs then report back.
  • I think I could add documentation, I know the API very well. Are better docs a higher priority than additional functionality, i.e. MCF API, cycling through dates by filtering on each date, etc.

@jreback
Copy link
Contributor

jreback commented Aug 10, 2013

I think basic docs in what's in their now are needed
I think the docs should go in remote_data.rst over io.rst
but could be debatable

more functionality as useful

@tshauck
Copy link
Contributor Author

tshauck commented Aug 10, 2013

Am I reading this correctly (first time using this)... that the test was not skipped since they aren't listed:

$ ci/print_skipped.py /tmp/nosetests.xml
SKIPPED TESTS:
---------------------------------------------------
#1 nose.failure.Failure.runTest: no clipboard found
#2 nose.failure.Failure.runTest: 
#3 nose.failure.Failure.runTest: skipping this for now
Done. Your build exited with 0.

@jreback
Copy link
Contributor

jreback commented Aug 10, 2013

I dont' think this library is setup correctly for testing/missing deps

  • need to add all deps to ci/requirements-2.7 (and 3.2); these are the full deps builds; its not even testing it right now
  • in pandas.io.ga set a flag if all of the deps are met or not, look at pandas/core/expressions.py for how numexpr is handle. if the deps are not installed, then calling a read_ga should raise an appropriate exception with a message that deps are not installed
  • point to the deps in optional section of install.rst
  • I am not sure of the API now, but what is the entry point now? e.g. should be read_ga (or something like that)
  • most of the I/O API stuff goes in pandas/io/api.py (e.g. it is loaded on default - this will fail right now), but I am not sure that this should be loaded by default anyhow, any thoughts?

seems deps are similar to BigQuery: #4140; this has not been merged yet, but need to add them for ga in any event (and there might be certain deps that are necessary ga that are not for BigQuery or vice versa)

@cpcloud
Copy link
Member

cpcloud commented Aug 11, 2013

minor correction full deps builds are 2.7, 3.2, and 3.3

@jreback
Copy link
Contributor

jreback commented Aug 23, 2013

this looks good...can you squash down to a single commit? otherwise ready 2 go?
@cpcloud ?

@tshauck
Copy link
Contributor Author

tshauck commented Aug 25, 2013

I think it's ready to go... I squashed the commit.

@jreback
Copy link
Contributor

jreback commented Aug 25, 2013

@tshauck

there are still some issues with this. It is not clear if travis is even testting this; at the very least it should show that its skipping the tests (which its not), as I KNOW that it doesn't have the deps installed.

There seem to be many more deps that we don't have installed, just from a quick try:

  • httplib2
  • urlib3
  • apiclient
  • oauth2client
  • google-api-python-client (which you had added to the requirements)

in the raise nose.SkipTest at the top of test_ga.py, can you put the exeption messages into the skip

e.g.

try:
   import stuf
except (Exception) as detail:
   raise nose.SkipTest(detail)

then can see if its skipping and why

doesn't google-api install all of these deps when it is installed? if not do you know why not?

@tshauck
Copy link
Contributor Author

tshauck commented Aug 31, 2013

@jreback

Installing google-api-python-client takes care of everything in your list except urllib3. Although I'm not seeing where you're getting that as a requirement?

I'm going to push the detailed exception change now and see what travis says.

@jreback
Copy link
Contributor

jreback commented Sep 21, 2013

@tshauck can you update ci/print_versions.py so that it prints the versions of these modules that are installed?

I am still not clear if this is actually testing properly

@jtratner
Copy link
Contributor

please update pandas/util/show_versions.py instead (recently moved the version info)

@jtratner
Copy link
Contributor

uh, well if it exists :P

@jreback
Copy link
Contributor

jreback commented Sep 21, 2013

@tshauck fyi...I added a Google Analytics label for issues..

@jreback
Copy link
Contributor

jreback commented Oct 2, 2013

@tshauck can you rebase and squash?

@tshauck
Copy link
Contributor Author

tshauck commented Oct 5, 2013

@jreback I squashed it down to the original commit...

@jreback
Copy link
Contributor

jreback commented Oct 5, 2013

why are the imports different among the 3 requirements files? what is glags? I still don't see it running any tests (or skipping)

@jreback
Copy link
Contributor

jreback commented Oct 11, 2013

@tshauck pls rebase this on master. BigQuery was merged which used google-api (so you can take out from print_versions). install.rst may need updating as well.

Also, pls put message on the SkipTests that skip because of missing deps or invalid authentication.

Thanks

@jtratner
Copy link
Contributor

After this is rebased, might want to ask one of the GBQ authors to test and
make sure this doesn't break anything for them.

@jreback
Copy link
Contributor

jreback commented Oct 14, 2013

@tshauck can you rebase?

@tshauck
Copy link
Contributor Author

tshauck commented Oct 14, 2013

@jreback Will do when I have time.

@jreback
Copy link
Contributor

jreback commented Oct 14, 2013

@tshauck gr8 thanks!

@jreback jreback closed this Jan 3, 2014
@tshauck tshauck deleted the have_profile_id branch January 3, 2014 20:45
@jreback
Copy link
Contributor

jreback commented Jan 3, 2014

@tshauck hmm...this got closed and can't reopen for some reason.

@jtratner
Copy link
Contributor

jtratner commented Jan 3, 2014

It's because the branch was deleted...

@tshauck tshauck restored the have_profile_id branch January 3, 2014 21:49
@tshauck
Copy link
Contributor Author

tshauck commented Jan 3, 2014

@jreback , @jtratner

undeleted, sorry, thought you closed this

@jreback jreback reopened this Jan 3, 2014
@jreback
Copy link
Contributor

jreback commented Jan 3, 2014

nah was changing versions and some issues for accident closed

@jreback
Copy link
Contributor

jreback commented Feb 16, 2014

@tshauck progress on this?

need to make sure that the ga calls are skipping appropriately if deps are not installed
(and of course since no authorization should skip too)

if you can validate those...and assuming you can test this locally we can get it in

@jreback
Copy link
Contributor

jreback commented Feb 16, 2014

realted to #3507

@tshauck
Copy link
Contributor Author

tshauck commented Feb 17, 2014

@jreback Progress had stalled, due to the testing issues mentioned in #3507 as well as me assuming I had to get the tests working with GA, which I wasn't sure how to do. I'll try to pick this back up.

I've been using the modification locally for a while w/o issue.

@jreback
Copy link
Contributor

jreback commented Feb 17, 2014

gr8

important that the tests skip properly of the deps are not installed
and also skip (with a different message when authentication missing or fails)
if possible can we get testing credentials ? I can encode them on Travis so that they do not appear anywhere know code (it's passed as a secure env variable)
then could put all the tests on Travis

lmk

@jreback
Copy link
Contributor

jreback commented Mar 9, 2014

@tshauck how's this coming?

@jreback jreback modified the milestones: 0.15.0, 0.14.0 Mar 28, 2014
@jreback
Copy link
Contributor

jreback commented Apr 5, 2014

@tshauck ?

@jreback
Copy link
Contributor

jreback commented Jul 6, 2014

closing this. pls resubmit when ready.

@jreback jreback closed this Jul 6, 2014
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.

4 participants