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

chore: Update frozen Python dependencies #26944

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Feb 1, 2024

SUMMARY

A rehash of #26913, though with less restrictive additional constraints.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@john-bodley john-bodley force-pushed the john-bodley--fix-pip-compile-multi branch from 27948df to 40dfb5e Compare February 1, 2024 01:04
Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - oh btw was hoping you would reopen on apache so I can help fix the integration tests failing. I think from this point on I'm always going to work on the main fork as there's only advantages it seems

@mistercrunch
Copy link
Member

Looks like some proxy object isn't initializing as expected, that stuff is a bit foreign to me, but can help fixing the issue if needed.

@john-bodley john-bodley force-pushed the john-bodley--fix-pip-compile-multi branch 3 times, most recently from 0c02fe5 to e19dfe5 Compare February 2, 2024 22:42
@john-bodley john-bodley force-pushed the john-bodley--fix-pip-compile-multi branch from e19dfe5 to f7c7f5a Compare February 2, 2024 23:05
@john-bodley
Copy link
Member Author

I'm really rather perplexed as to why the tests are failing as Superset (especially the tests) can be rather magical at times. The problem I'm facing is

    @mock.patch("uuid.uuid4", return_value=UUID)
    def test_events(self, mock_uuid4):
        app._got_first_request = False
>       async_query_manager.init_app(app)
E       AttributeError: 'NoneType' object has no attribute 'init_app'

where the async_query_manager is a local proxy to the factory instance. Per the code the factory instance (self._async_query_manager) is initialized here which requires the GLOBAL_ASYNC_QUERIES feature flag to be enabled—which is not the case for this test.

I've seen other tests which explicitly bind the app to the factor, i.e., adding

async_query_manager_factory.init_app(app)

fixes the test. The issues however are:

  • I don't understand how this test actually passes on master (irrespective of the pinned dependencies).
  • Fixing the test is necessary but not sufficient for CI to pass, i.e., other tests fail.

@betodealmeida , @michael-s-molina, or @villebro would you happen to have any insights with regards to these "quirks"? I did look into seeing whether Werkzeug's LocalProxy logic had changed, i.e., if a LocalProxy of None previously wasn't None, but downloading to the previously pinned version (differing at the minor level) didn't resolve the issue.

@mistercrunch
Copy link
Member

mistercrunch commented Feb 2, 2024

Yeah I spent a moment looking at this and was confused too, something like a non-deterministic sequence of events that's affected by a library upgrade(?) I looked at the libs we're bumping to try and understand any that could have something to do with this, but couldn't think of any while reviewing the list.

@john-bodley
Copy link
Member Author

@mistercrunch regarding,

I looked at the libs we're bumping to try and understand any that could have something to do with this, but couldn't think of any while reviewing the list.

I did the same and couldn't find anything abnormal. I'm still unsure whether there are gremlins in our system and that act of bumping the Python dependencies is actually surfacing these (given that I can't fully grok how the failed test(s) actually pass on master).

@mistercrunch
Copy link
Member

Ha! Yeah I thought about bissecting the library bumps and then realized life is too short for this nonsense.

Can python just support diamond dependencies!? Why can npm do it and not python/pip? Is it that hard of a problem? There's gotta be a PEP for it somewhere. Python 4 here we go!

from __future__ import diamond_dependencies

@mistercrunch
Copy link
Member

I still get nightmares from the python 3 migrations. Waking up in sweat stressing over breaking thousands of 2.7 installs.

mistercrunch added a commit that referenced this pull request Mar 6, 2024
both @john-bodley and I have tried to fix the pip-compile-multi flow
that we need to manage our complex network of python dependencies.

Last attempt was here -> #26944

Here I'm reopening the PR on the main fork so we can collaborate on it
more easily.
@mistercrunch
Copy link
Member

closing in favor of #27404, where we can collab on the main fork

mistercrunch added a commit that referenced this pull request Mar 14, 2024
both @john-bodley and I have tried to fix the pip-compile-multi flow
that we need to manage our complex network of python dependencies.

Last attempt was here -> #26944

Here I'm reopening the PR on the main fork so we can collaborate on it
more easily.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants