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

Help make test dependent on randint compatible with python 3.12 #8119

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

hmaarrfk
Copy link
Contributor

It seems that python 3.12 is a little stricter on the inputs of randint.

I hope this helps

In [1]: import random

In [2]: random.randint(10, 10/2)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In[2], line 1
----> 1 random.randint(10, 10/2)

File ~/miniforge3/envs/py12/lib/python3.12/random.py:336, in Random.randint(self, a, b)
    332 def randint(self, a, b):
    333     """Return random integer in range [a, b], including both end points.
    334     """
--> 336     return self.randrange(a, b+1)

File ~/miniforge3/envs/py12/lib/python3.12/random.py:312, in Random.randrange(self, start, stop, step)
    309     raise ValueError("empty range for randrange()")
    311 # Stop argument supplied.
--> 312 istop = _index(stop)
    313 width = istop - istart
    314 istep = _index(step)

TypeError: 'float' object cannot be interpreted as an integer

Copy link

pytorch-bot bot commented Nov 16, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/8119

Note: Links to docs will display an error until the docs builds have been completed.

❌ 14 New Failures, 18 Unrelated Failures

As of commit a4bc6b3 with merge base dd66a9d (image):

NEW FAILURES - The following jobs have failed:

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@NicolasHug
Copy link
Member

Thanks for the PR @hmaarrfk .

We don't support 3.12 yet but when the time comes, we'll be happy to re-consider this PR.

I genuinely hope that this is not here to stay TBH

>>> random.randint(0, 4/2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/nicolashug/.miniconda3/envs/fuck/lib/python3.12/random.py", line 336, in randint
    return self.randrange(a, b+1)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/nicolashug/.miniconda3/envs/fuck/lib/python3.12/random.py", line 312, in randrange
    istop = _index(stop)
            ^^^^^^^^^^^^
TypeError: 'float' object cannot be interpreted as an integer

@hmaarrfk
Copy link
Contributor Author

thanks for looking at this so quickly and taking the time to test on your machine.

this seems intentional. see notes about automatic conversation.

https://docs.python.org/3/library/random.html#functions-for-integers

integer division resolves this issue easily.
the PR is 8 characters that go in the right direction toward 3.12 support without hurting other versions in any way. it is contained in the test suite, and it is obvious from inspection that the type of the inputs to the division operator are integers.

I mostly just wanted to help given that I had tried to see how far along python 3.12 one could go. I'm not asking you to accelerate your timeline in any way.

take it or leave it I guess.

@hmaarrfk
Copy link
Contributor Author

Just a kind ping hoping you can consider the introduction of a total of 10 characters.

@NicolasHug
Copy link
Member

NicolasHug commented Feb 26, 2024

Thank you for the ping @hmaarrfk . We'll merge the PR in due time, when 3.12 support becomes official for Pytorch and torchvision.

I'm sure you're experienced enough to know that the complexity, debt and criteria for inclusion of a PR doesn't correlate with its number of characters.

@NicolasHug NicolasHug mentioned this pull request Mar 4, 2024
@NicolasHug NicolasHug added this to Must in 0.18 TODOs Mar 12, 2024
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR and for your patience @hmaarrfk. We'll want to release for 3.12 in the next release, so let's get that in now. I'll merge when CI is green.

@NicolasHug NicolasHug moved this from Must to Ideally in 0.18 TODOs Mar 13, 2024
@NicolasHug NicolasHug moved this from Ideally to approved in 0.18 TODOs Mar 13, 2024
@NicolasHug
Copy link
Member

Failures are unrelated, let's merge

@NicolasHug NicolasHug merged commit e00f4e6 into pytorch:main Mar 13, 2024
9 of 31 checks passed
@NicolasHug NicolasHug moved this from approved to done in 0.18 TODOs Mar 13, 2024
facebook-github-bot pushed a commit that referenced this pull request Mar 20, 2024
….12 (#8119)

Summary: Co-authored-by: Nicolas Hug <[email protected]>

Reviewed By: vmoens

Differential Revision: D55062798

fbshipit-source-id: e24a223a82eda5ce9ad5ea816d04ab2725f253ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants