Conversation
Sorry, something went wrong.
… which will cause a DefaultCredentialsError.
rosbo
left a comment
There was a problem hiding this comment.
Could you add a test cases for this code path?
Sorry, something went wrong.
|
@rosbo I thought about that but this change is only a logging/print() change so I'm not sure how to effectively test for it? Should I mock print? |
Sorry, something went wrong.
|
True, it's probably fine as-is. :) |
Sorry, something went wrong.
ifigotin
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
Sorry to revive this PR, but I realized on testing this that the print message was appearing twice and the stack trace looked like the monkeypatching was monkeypatching the already monkeypatched (tongue-twister :) method. I believe this happens because during import of bigquery/gcs the GCPLoader imports sitecustomize (which monkeypatches it the first time) and then calls kaggle_gcp.init_bigquery to return the module, which monkeypatches it again. To be safe, I added a check during the monkeypatching to avoid wrapping an already monkeypatched method (bigquery.Client() or storage.Client.init). The unit tests I added repro this, and was able to confirm it worked correctly in the Kernel. |
Sorry, something went wrong.
ifigotin
left a comment
There was a problem hiding this comment.
LGTM
I assume this change doesn't significantly slow things down.
Sorry, something went wrong.
… which will cause a DefaultCredentialsError. Additionally, it ensures that we only monkeypatch the desired module methods once, to avoid double wrapping the methods.
b/138330551