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

Bug introduced in v3.12.0 #1838

Closed
jacek-jablonski opened this issue Feb 29, 2024 · 8 comments · Fixed by #1848
Closed

Bug introduced in v3.12.0 #1838

jacek-jablonski opened this issue Feb 29, 2024 · 8 comments · Fixed by #1848
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@jacek-jablonski
Copy link

jacek-jablonski commented Feb 29, 2024 •
edited

Hi,

I think a bug was introduced in v3.12.0, it this commit: 3645e32#diff-80c3339e876a6651c796df9b6621e6c8fba7d8b168f65239753398a0441b06a9R1345

isinstance(someinteger, object) called like that is always True, because int is an instance of an object:

>>> isinstance(3, object)
True

As a result, the timeout is always changed to None at this point. This causes timeout to not be respected when for example QueryJob.exception method is called.
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Feb 29, 2024
@Linchin
Copy link
Contributor

Linchin commented Mar 5, 2024

Thank you @jacek-jablonski for raising this issue! To ensure that I can reproduce the exact problem, could you provide a code snippet? This way I can add a unit test for this case.

@Linchin Linchin added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Mar 5, 2024
@jacek-jablonski
Copy link
Author

jacek-jablonski commented Mar 6, 2024 •
edited

Hi @Linchin,

here is a code snippet:

import concurrent.futures

import pytest
from google.cloud import bigquery


TEST_SLEEP_QUERY = """
BEGIN
    DECLARE DELAY_TIME DATETIME;
    DECLARE WAIT STRING;

    SET WAIT = 'TRUE';
    SET DELAY_TIME = DATETIME_ADD(CURRENT_DATETIME, INTERVAL {seconds} SECOND);

    WHILE WAIT = 'TRUE' DO
        IF (DELAY_TIME < CURRENT_DATETIME) THEN
            SET WAIT = 'FALSE';
        END IF;
    END WHILE;
END;
"""


def test_query_timeout():
    client = bigquery.Client()
    query_job = client.query(TEST_SLEEP_QUERY.format(seconds=10))

    with pytest.raises(concurrent.futures.TimeoutError):
        query_job.exception(timeout=1)

@Linchin
Copy link
Contributor

Linchin commented Mar 6, 2024

Thank you @jacek-jablonski. I tried out the sample, and although the value of self._done_timeout was indeed wiped out into None, but it doesn't seem to do anything - if I set query_job.exception(timeout=1), I get a timeout error; if I set query_job.exception(timeout=100), I don't get any error. Am I missing something here? What is the expected behavior?

@jacek-jablonski
Copy link
Author

Hi @Linchin,

for me, this test fails on v3.18.0:

======================================================= FAILURES =======================================================
__________________________________________________ test_query_timeout __________________________________________________

    def test_query_timeout():
        client = bigquery.Client()
        query_job = client.query(TEST_SLEEP_QUERY.format(seconds=10))

>       with pytest.raises(concurrent.futures.TimeoutError):
E       Failed: DID NOT RAISE <class 'TimeoutError'>

test_bq.py:28: Failed
=============================================== short test summary info ================================================
FAILED test_bq.py::test_query_timeout - Failed: DID NOT RAISE <class 'TimeoutError'>
================================================== 1 failed in 12.20s ==================================================

and passes on v3.11.2:

test_bq.py .                                                                                                     [100%]

================================================== 1 passed in 3.12s ===================================================

@Linchin
Copy link
Contributor

Linchin commented Mar 7, 2024

I'm still unable to reproduce the issue - could you let me know which version of python you are using, and possibly the versions of your dependencies?

@jacek-jablonski
Copy link
Author

I recorded a screencast:

Nagranie.z.ekranu.2024-03-8.o.09.21.23.mp4

Below some more information.

Python version: 3.11.7
Pip freeze when v3.18.0 is installed:

cachetools==5.3.3
certifi==2024.2.2
charset-normalizer==3.3.2
google-api-core==2.17.1
google-auth==2.28.1
google-cloud-bigquery==3.18.0
google-cloud-core==2.4.1
google-crc32c==1.5.0
google-resumable-media==2.7.0
googleapis-common-protos==1.62.0
grpcio==1.62.0
grpcio-status==1.62.0
idna==3.6
iniconfig==2.0.0
packaging==23.2
pluggy==1.4.0
proto-plus==1.23.0
protobuf==4.25.3
pyasn1==0.5.1
pyasn1-modules==0.3.0
pytest==8.0.2
python-dateutil==2.9.0.post0
requests==2.31.0
rsa==4.9
six==1.16.0
urllib3==2.2.1

Pip freeze when v3.11.2 is installed:

cachetools==5.3.3
certifi==2024.2.2
charset-normalizer==3.3.2
google-api-core==2.17.1
google-auth==2.28.1
google-cloud-bigquery==3.11.2
google-cloud-core==2.4.1
google-crc32c==1.5.0
google-resumable-media==2.7.0
googleapis-common-protos==1.62.0
grpcio==1.62.0
grpcio-status==1.62.0
idna==3.6
iniconfig==2.0.0
packaging==23.2
pluggy==1.4.0
proto-plus==1.23.0
protobuf==4.25.3
pyasn1==0.5.1
pyasn1-modules==0.3.0
pytest==8.0.2
python-dateutil==2.9.0.post0
requests==2.31.0
rsa==4.9
six==1.16.0
urllib3==2.2.1

@Linchin
Copy link
Contributor

Linchin commented Mar 8, 2024

Thanks @jacek-jablonski! I'm finally able to reproduce it in python 3.11. Let me take a deeper look into it.

@Linchin
Copy link
Contributor

Linchin commented Mar 8, 2024

It's also flaky with python 3.11 - sometimes the exception is raised, sometimes it's not. Regardless I will fix the type check first.

@Linchin Linchin mentioned this issue Mar 8, 2024
Merged
gcf-merge-on-green bot pushed a commit that referenced this issue Mar 11, 2024
@Linchin
fix: correct type checking (#1848)
2660dbd 
Correct the way we check whether `self._done_timeout` is an instance of `object` class or not.

Fixes #1838 🦕
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api: bigquery Issues related to the googleapis/python-bigquery API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: correct type checking Linchin/python-bigquery
2 participants
@Linchin @jacek-jablonski