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.

Sign up for GitHub

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

Jump to bottom

Add monitoring missing dependency: pandas #5100

Closed
wants to merge 5 commits into from
Closed

Add monitoring missing dependency: pandas #5100

wants to merge 5 commits into from

Conversation

kabute
Copy link

@kabute kabute commented Mar 23, 2018

Add missing pandas dependency as per:

google/cloud/monitoring/_dataframe.py
google/cloud/monitoring/client.py
google/cloud/monitoring/query.py

@kabute kabute requested a review from waprin as a code owner March 23, 2018 13:29
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 23, 2018
@kabute
Copy link
Author

kabute commented Mar 23, 2018

(Fixed unit testing for pandas)

@tswast
Copy link
Contributor

tswast commented Mar 23, 2018

Pandas should probably be an "extras" as was done here: #4354

@kabute
Copy link
Author

kabute commented Mar 25, 2018

Given that client query will break (Pandas seems to be a hard dependency), can I suggest changes to?

  • Add the dependency to the README (indicate users this fact).
  • Fix unit testing (won't pass, including coverage) and put pandas tests on its own file.

@tswast tswast requested a review from theacodes March 26, 2018 16:25
@theacodes
Copy link
Contributor

It's fine with me if this needs to directly depend on Pandas. Is the query method the most commonly used method here?

@tseaver
Copy link
Contributor

tseaver commented Mar 26, 2018

@jonparrott It looks like pandas should only be a conditional dependency: folks who want to use the rest of the package, but not use query.as_dataframe, shouldn't need to install that rather large dependency.

@theacodes
Copy link
Contributor

Okay so it's not needed to query, just need to do query.as_dataframe. In which case, yeah, this should just be optional.

@kabute
Copy link
Author

kabute commented Apr 8, 2018

Then extras is probably the best way to go (and add the conditional back on the unit testing). What do you think?

@kabute
Copy link
Author

kabute commented Apr 9, 2018

Added pandas as optional (and documented how to install it).

theacodes
theacodes reviewed Apr 9, 2018
View reviewed changes
monitoring/setup.py Outdated
@@ -33,6 +33,7 @@
'google-api-core<2.0.0dev,>=0.1.1',
]
extras = {
'optional': ['pandas>=0.22.0'],

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

theacodes
theacodes suggested changes Apr 9, 2018
View reviewed changes
monitoring/README.rst

**Pandas:**

Pandas is an optional dependency that is needed if *query.as_dataframe* is used.

This comment was marked as spam.

Sign in to view

monitoring/README.rst

To install pandas package run:

$ pip install --upgrade .[pandas]

This comment was marked as spam.

Sign in to view

monitoring/tests/unit/test__dataframe.py
@@ -14,14 +14,13 @@

try:
import pandas
except ImportError:
except ImportError: # pragma: NO COVER

This comment was marked as spam.

Sign in to view

monitoring/tests/unit/test__dataframe.py
HAVE_PANDAS = False
else:
HAVE_PANDAS = True # pragma: NO COVER
else: # pragma: NO COVER

This comment was marked as spam.

Sign in to view

monitoring/tests/unit/test__dataframe.py
self.assertIsNone(dataframe.index.name)
self.assertIsInstance(dataframe.index, pandas.DatetimeIndex)


class Test__sorted_resource_labels(unittest.TestCase):
class Test__sorted_resource_labels(unittest.TestCase): # pragma: NO COVER

This comment was marked as spam.

Sign in to view

@tseaver tseaver added packaging api: monitoring Issues related to the Cloud Monitoring API. labels Apr 18, 2018
@theacodes
Copy link
Contributor

superseded by #5212

@theacodes theacodes closed this Apr 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theacodes theacodes theacodes requested changes

@waprin waprin Awaiting requested review from waprin

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

Assignees
No one assigned
Labels
api: monitoring Issues related to the Cloud Monitoring API. cla: yes This human has signed the Contributor License Agreement. packaging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kabute @tswast @theacodes @tseaver @googlebot