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

CI: Catch importwarning _SixMetaPathImporter.find_spec #48278

Closed
wants to merge 10 commits into from

Conversation

phofl
Copy link
Member

@phofl phofl commented Aug 26, 2022

Lets see if this helps somehow

I can't see how this could be related to our s3fs and aiobotocore pins. The following is the version diff from before and after the pin:

                 Name_old     Name_new Version_old Version_new
3             aiobotocore  aiobotocore       2.4.0       1.4.2
26                  boto3        boto3     1.24.59    1.17.106
27               botocore     botocore     1.27.59    1.20.106
76                 fsspec       fsspec    2022.7.1    2021.7.0
77                  gcsfs        gcsfs    2022.7.1    2021.7.0
98   google-cloud-storage          NaN       2.5.0         NaN
119              jmespath     jmespath       1.0.1      0.10.0
286                  s3fs         s3fs       0.6.0    2021.7.0
287            s3transfer   s3transfer       0.6.0       0.4.2

Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

I wouldn't be opposed to just adding this to filterwarnings in pyproject.toml since pandas doesn't explicitly use six so it should be from one of the dependencies: benjaminp/six#341

@phofl
Copy link
Member Author

phofl commented Aug 26, 2022

Yes, definitely. Its a bit hard to find out from which one though.

Just want to see if this helps. Can add to pyproject.toml afterwards

@phofl
Copy link
Member Author

phofl commented Aug 26, 2022

Added to pyproject.toml, lets see if it works. Can not reproduce the warning locally

@mroeschke mroeschke added CI Continuous Integration Upstream issue Issue related to pandas dependency labels Aug 26, 2022
@phofl
Copy link
Member Author

phofl commented Aug 27, 2022

It looks like filterwarnings does not work well together with assert_produces_warning. Could not get tests to pass locally when filtering the warning with pytest and checking for no warning

@phofl
Copy link
Member Author

phofl commented Aug 27, 2022

cc @mroeschke

This is greenish now. Check is ugly though

@lithomas1
Copy link
Member

So I did a little debugging, and the warning is coming from botocore(they vendor six), and it looks like the pinning downgraded botocore, cuasing this warning to come back.

Copy link
Member

@lithomas1 lithomas1 left a comment

Choose a reason for hiding this comment

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

This is a symptom of pinning aiobotocore(see comments in #48272)

@phofl
Copy link
Member Author

phofl commented Aug 28, 2022

Yeah but this is necessary, see #48272 (comment)

@mroeschke
Copy link
Member

mroeschke commented Aug 29, 2022

I would be okay with this PR as is as a temporary fix if pinning botocore doesn't work #48291 (comment). (It is really strange behavior that a s3fs code path is being hit when it should have nothing to do with it?)

I would prefer not to pin s3fs higher than our minimum version if possible (and generally not have our dep files have different pins in general).

@lithomas1
Copy link
Member

The thing is, this does have to do with it. The way I understand it is s3fs pins aiobotocore which pins down botocore. As I mentioned, pinning aiobotocore in the other PR is pinning down botocore to an old version that vendors an old six doesn't address the warning.

So I would be -1 on this, since this ImportWarning is telling us that pinning aiobotocore was not the right move.

@phofl
Copy link
Member Author

phofl commented Aug 29, 2022

S3fs is not pinning aiobotocore, but we have to pin since the old s3fs is not compatible with aiobotocore > 2.0. pinning botocore will most likely cause non solvable environments

i think pinning aiobotocore is the right move here in a sense, not having a higher s3fs minimum requirement is the actual issue. The old s3fs is not compatible with 3.10

can we bump this in the rc? Or do we have to follow a certain version Schema there?

@mroeschke
Copy link
Member

can we bump this in the rc? Or do we have to follow a certain version Schema there?

As mentioned in #48291, I would be +1 to simply bumping s3fs in the 1.5.0rc. I have been following a "min version that has been released one year prior" rule of thumb for most optional dependencies.

@phofl
Copy link
Member Author

phofl commented Aug 29, 2022

Thx, closing in favour of #48299

@phofl phofl closed this Aug 29, 2022
@phofl phofl deleted the import_warning branch August 29, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Upstream issue Issue related to pandas dependency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants