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

Update collections from Python 3.11.2 #4876

Merged
merged 3 commits into from
Apr 28, 2023
Merged

Conversation

Phosphorescentt
Copy link
Contributor

Also added an extra couple of lines to the test just to make sure everyting's working as intended.

Let me know if this looks good or if any changes are needed :)

Lib/test/test_ordered_dict.py Outdated Show resolved Hide resolved
@Phosphorescentt
Copy link
Contributor Author

Checked this implementation against the old one and it makes the test pass without causing any other ones to fail

@DimitrisJim
Copy link
Member

DimitrisJim commented Apr 16, 2023

Thanks! Do you mind updating all the files in collections? As far as I can tell, this just updates the __init__.py file, right? (feel free to ignore if you updated all and theres just no diff) It would be best if the PR updated them all so we can have all files be in sync. (See #4564 for reference here).

We can always merge and update the others later but this way we get two birds with one stone.

@Phosphorescentt
Copy link
Contributor Author

As far as I can tell there's nothing to be changed in either collections/abc.py or collections/defaultdict.py so I think we're good? Unless I've made another blunder which is very possible 😅

@DimitrisJim
Copy link
Member

DimitrisJim commented Apr 16, 2023

Copying __init__.py over from the 3.11 branch (specifically 3.11.2) shows a couple more changes that have been made to it (some in OrderedDicts __reduce__, some shifting of methods in Counter and some tiny comment changes). Maybe you've copied from an older version?

Note: One of these is a FIXME added by us, don't remove that (basically, add it back in after you copy __init__.py over) since its still required 😄

So, I think the best approach now is to include those changes from 3.11 here, rename the PR to Update collections from Python 3.11.2 and we can see if the test suite picks anything else up.

Thanks again for opening, feel free to ask if anything gets confusing.

@Phosphorescentt Phosphorescentt changed the title Updated SimpleLRUCacheTests.test_pop to pass Update collections from Python 3.11.2 Apr 16, 2023
@Phosphorescentt
Copy link
Contributor Author

Made the suggested changes. Thanks for all the help :)

Didn't realise so much of the ethos of this project is about staying in line with the CPython implementations (which of course reflecting on it makes total sense)!

@Phosphorescentt
Copy link
Contributor Author

Just checking the tests now and it seems like actually these changes fix another test that was failing!

___________ PurePythonOrderedDictWithSlotsCopyingTests.test_copying ____________
Unexpected success

If we're happy that this test should be passing then I'll go ahead and take away the decorator :)

@DimitrisJim
Copy link
Member

I'm going to guess it most likely isn't happy with 8e55919 which seems kinda odd considering your output. Since the runner's thirst for success must be appeased we should rollback that commit and give it a second try to make sure.

@Phosphorescentt
Copy link
Contributor Author

I think that's done? Still working on my git-fu

@Phosphorescentt
Copy link
Contributor Author

Phosphorescentt commented Apr 16, 2023

Had a dig through the CI logs and it seems like the following tests are failing in a number of test classes:

  • test_copying
  • test_pickle_recursive
  • test_yaml_linkage
  • test_reduce_not_too_fat

I can't seem to replicate the errors with test_pickle_recursive, test_yaml_linkage, test_reduce_not_too_fat, but on my end test_copying failing just seems to be due to the same decorator as in 8e55919. (I guess this makes sense seeing as the mac os tests go through fine and I'm on mac os)

@DimitrisJim
Copy link
Member

DimitrisJim commented Apr 16, 2023

Okay, failures do make sense, they all are a result of the new __reduce__ for OrderedDict (which calls __setstate__ which is nowhere to be found). This sometimes happens, library is updated, uses a feature we might not have, tests that worked fine with the old implementation now fail.

You can now mark these as expected failures and I should probably see what is the deal with __setstate__.

@Phosphorescentt
Copy link
Contributor Author

Phosphorescentt commented Apr 16, 2023

Before I add those decorators, would it be worth removing all of the decorators and rerunning the workflows just so everything's fully up to date and we can see which tests are going through according to the CI?

Also seems like an issue with __getstate__ too? Not sure why the tests are only failing on ubuntu and not on mac os though? The code for __reduce__ makes a call to __getstate__() immediately, but, like you, I can't find a definition for __getstate__() on anything dictionary related yet the tests still pass.

@DimitrisJim
Copy link
Member

DimitrisJim commented Apr 16, 2023

Not sure why the tests are only failing on ubuntu and not on mac os though?

Don't think they are executed on Mac. These are platform independent (don't really test any platform specific things) so having them executed only on Ubuntu (fastest runner) is a solution to cut down on unnecessary execution time.

@Phosphorescentt
Copy link
Contributor Author

Not sure why the tests are only failing on ubuntu and not on mac os though?

Don't think they are executed on Mac. These are platform independent (don't really test any platform specific things) so having them executed only on Ubuntu (fastest runner) is a solution to cut down on unnecessary execution time.

Fair enough - makes sense. Still not sure why this test passes on my laptop though.

Before I add those decorators, would it be worth removing all of the decorators and rerunning the workflows just so everything's fully up to date and we can see which tests are going through according to the CI?

Opinions on this idea? Probably worth making sure that we know which tests are now passing and/or failing?

@fanninpm
Copy link
Contributor

Before I add those decorators, would it be worth removing all of the decorators and rerunning the workflows just so everything's fully up to date and we can see which tests are going through according to the CI?

Opinions on this idea? Probably worth making sure that we know which tests are now passing and/or failing?

It would be a good idea to remove RustPython-specific @unittest.skip() decorators to see if they still need to be skipped. The @unittest.expectedFailure decorators don't need to be removed.

@fanninpm
Copy link
Contributor

I can't seem to replicate the errors with test_pickle_recursive, test_yaml_linkage, test_reduce_not_too_fat

I believe you can use the following command to test your changes locally:

# this runs all of the tests (not necessary if you know which test is affected)
cargo run --release --features ssl,jit -- -m test -j 1 -u all --slowest --fail-env-changed -v
# this runs only the test suite named "test_whatever" (usually located at `Lib/test/test_whatever.py`)
cargo run --release --features ssl,jit -- -m test -j 1 -u all --slowest --fail-env-changed -v test_whatever

For a list of all resources and more command-line options, you can execute cargo run --release --features ssl,jit -- -m test -h.

@Phosphorescentt
Copy link
Contributor Author

Ahhhh thank you for this!

Just updated the decorators so now all the tests should pass the CI.

Copy link
Contributor

@fanninpm fanninpm left a comment

Choose a reason for hiding this comment

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

These @unittest.expectedFailure decorators aren't in the CPython source, so we should mark them as RustPython-specific.

Lib/test/test_ordered_dict.py Show resolved Hide resolved
Lib/test/test_ordered_dict.py Show resolved Hide resolved
Lib/test/test_ordered_dict.py Show resolved Hide resolved
Lib/test/test_ordered_dict.py Show resolved Hide resolved
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

I found collections/__init__ and test_ordered_dict is not fully updated without comments.
Attaching a commit to resolve them.

I am sorry, I was watching wrong version of CPython

Added extra `unittest.expectedFailure` decorators
@DimitrisJim
Copy link
Member

re-run of ci won't fly well with the recent windows ci changes it seems @Phosphorescentt can you rebase?

@youknowone youknowone merged commit 429d1d1 into RustPython:main Apr 28, 2023
13 checks passed
@youknowone
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants