googleapis / python-pubsub

Apache License 2.0
385 stars 200 forks source link

chore: Remove restriction that protobuf should be less than 5 #1193

Closed mukund-ananthu closed 2 months ago

mukund-ananthu commented 2 months ago

I see that an upper bound of 5 was set on the protobuf library in https://github.com/googleapis/python-pubsub/pull/762/commits/23ce35e92c68f561fd2208439479983f0f8344c8

I do not find any documentation on reasons for keeping an upper bound. Proposing removing the upper bound in this PR, but also checking if there are any reasons we would not want to do this from @parthea who would have historical context on this.

Fixes #1192 🦕

BEGIN_COMMIT_OVERRIDE fix: allow Protobuf 5.x END_COMMIT_OVERRIDE

mukund-ananthu commented 2 months ago

@parthea PTAL when free.

jeffsawatzky commented 2 months ago

I would very much like to have this fixed as well, for similar reasons to #1192 @mukund-ananthu / @parthea this is currently blocking me due to dependency hell. If you could remove this restriction (or loosen it to <6.0.0dev for now) it would make things much easier. Thanks!

parthea commented 2 months ago

Also update testing/constraints-3.7.txt

Change

https://github.com/googleapis/python-pubsub/blob/0b8d7b91da7928e8f06379d19c616e7a6a411349/testing/constraints-3.7.txt#L10

to

protobuf==3.20.2
parthea commented 2 months ago

The system tests are failing due to an unrelated issue. See https://github.com/googleapis/python-pubsub/issues/created_by/app/flaky-bot

___________ test_subscriber_not_leaking_open_sockets[rest-rest-rest] ___________

publisher = 
topic_path_base = 'projects/<REDACTED>/topics/<REDACTED>'
subscription_path_base = 'projects/<REDACTED>/subscriptions/<REDACTED>'
cleanup = [(>, (), {'topic': 'projects/<REDACTED>/topics/<REDACTED>'})]
transport = 'rest'

    @pytest.mark.parametrize("transport", ["grpc", "rest"])
    def test_subscriber_not_leaking_open_sockets(
        publisher, topic_path_base, subscription_path_base, cleanup, transport
    ):
        # Make sure the topic and the supscription get deleted.
        # NOTE: Since subscriber client will be closed in the test, we should not
        # use the shared `subscriber` fixture, but instead construct a new client
        # in this test.
        # Also, since the client will get closed, we need another subscriber client
        # to clean up the subscription. We also need to make sure that auxiliary
        # subscriber releases the sockets, too.
        custom_str = "-not-leaking-open-sockets"
        subscription_path = subscription_path_base + custom_str
        topic_path = topic_path_base + custom_str
        subscriber = pubsub_v1.SubscriberClient(transport="grpc")
        subscriber_2 = pubsub_v1.SubscriberClient(transport="grpc")

        cleanup.append(
            (subscriber_2.delete_subscription, (), {"subscription": subscription_path})
        )
        cleanup.append((subscriber_2.close, (), {}))
        cleanup.append((publisher.delete_topic, (), {"topic": topic_path}))

        # Create topic before starting to track connection count (any sockets opened
        # by the publisher client are not counted by this test).
        publisher.create_topic(name=topic_path)

        current_process = psutil.Process()
>       conn_count_start = len(current_process.connections())

tests/system.py:471: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = psutil.Process(pid=397, name='py.test', status='running', started='16:34:59')
args = (), kwargs = {}

    @functools.wraps(fun)
    def inner(self, *args, **kwargs):
>       warnings.warn(msg, category=DeprecationWarning, stacklevel=2)
E       DeprecationWarning: connections() is deprecated and will be removed; use net_connections() instead
mukund-ananthu commented 2 months ago

@jeffsawatzky and @cglewis . The fix is submitted and available in the latest release 2.21.5