Add support for Python3.10 on testing Docker image

[humitos]

Requires: readthedocs/readthedocs-docker-images#174

[ericholscher]

Seems like these should probably be strings, but guessing that ship has sailed. Are they floats now?

[humitos]

I don't remember this setting being a different type than float. We are accepting floats and string in the config file, but if it's a string we are converting it to float. I found we have a test case for that https://github.com/readthedocs/readthedocs.org/blob/884132a5b55a6d5e7e70d8da4856f47df1d352d7/readthedocs/config/tests/test_config.py#L384-L399

So, I suppose that using string here will require more work.

... I just realized that I'm sleepy 😴 ...

This may be a problem, I guess. We will be comparing against 3.1 --which, it will work with because we are changing python: version: "3.10" to float as well ending up as 3.1, but it will eventually explode 💣

[humitos]

If we ever hit this problem, it would probably be in Python 3.50 because we already accept Python 3.5 😅

[astrojuanlu]

Is it really too late to change this? 🙈

[humitos]

@stsewd how much work would be to change python.version to be support float and string and use the string version in all our internal code? (config file, build environment, tests, settings, etc).

Also, we should change our documentation to de-motivate the usage of float on this config and re-write all the occurrences to use the string version.

[stsewd]

should be fine. We used to compare the versions as numbers to get the highest version for 3 and 2, but we use another method for that now.

[humitos]

As this doesn't affect us immediately, I don't want to block the upgrade to the new Docker image because of this problem. I opened this issue to track this change: #8361

[humitos]

Meh, it doesn't work when doing the build /bin/sh: 1: python3.1: not found

[humitos]

I pushed a commit to support "3.10" as string in python.version.

However, I had to made some decisions:

• using string in python.version is not cast to int/float anymore
• using "3.6" (as string) in the YAML keeps working for backward compatibility

This helps us to keep all the config files working as they were. However, I had to adapt our code to:

• add all the string versions for python.supported_versions in our settings file
• update some tests to not auto-cast python version
• allow python.supported_versions to use int, float, and string versions
• add a specific case for 3.10 when getting the python_interpreter

These changes are enough to make 3.10 work on our platform. I didn't find any issue with them yet, but it may be good to take a deeper look in the future and eventually remove the support for int/float values on python.version in the config file.

[astrojuanlu]

This doesn't include #8335 and therefore building an image with it fails. Perhaps a merge from master on this branch would be useful?

[humitos]

Perhaps a merge from master on this branch would be useful?

Done.

[humitos]

Perhaps a merge from master on this branch would be useful?

Done!

[astrojuanlu]

I was stopped by an outdated common: #8368

[stsewd]

Think we can just transform this to a string instead of handling strings and numbers, ver = str(self.python.version) if self.python.version is not None else None

[stsewd]

(this can be done when populating python.version instead of here is seems)

[humitos]

(this can be done when populating python.version instead of here is seems)

Here you mean by casting it as str()?

readthedocs.org/readthedocs/config/config.py

Lines 516 to 519 in 80b3c2c

	python['version'] = validate_choice(
	raw_python['version'],
	self.get_valid_python_versions(),
	)

If so, I'm not 100% convinced if it's good to change the type written by the user automatically --that problem led us here. I think we will face other issues silently. Are there other benefits besides the sugar syntax of this line?

[stsewd]

Here you mean by casting it as str()?

Yes

I think we will face other issues silently. Are there other benefits besides the sugar syntax of this line?

We don't need to take into consideration strings and numbers in all our code base, don't think this will cause problems, if you mean the users setting this value as 3.50, that will be cast as 3.5 already (float), transforming that to string leads to the same 3.5

[humitos]

I'm 👍🏼 on always using string internally. However, changing this to always use string internally requires some extra work. There are a bunch of tests to adapt and some internal functions to change and maybe re-think it's logic. I may consider this a refactor outside the scope of this PR and we can do this work in another one, probably.

If we want to support 3.10 and newer conda starting tomorrow after the deploy, I think we should merge this solution.

[stsewd]

I think the primary issue I'm expecting is a user who passes 3.10 as a float and it gets read as 3.1

That's a good question, looks like the yaml spec automatically cast those, so not much we can do, we can change all of our examples to always use strings

>>> yaml.safe_load(Path('test.yaml').open())
{'version': 3.1, 'version2': '3.10'}

version: 3.10
version2: "3.10"

[stsewd]

Fail the build if the user passes 3.10 instead of '3.10'?

I think that should be the case, and we should always use strings in our docs and maybe put a warning instead of magically changing 3.1 to 3.10

[ericholscher]

Yea, from the discussion, it seems like using floats for this is broken by design, and we need to systemically move to strings everywhere. I think we want to maintain backwards compat here though, so we should continue to accept the strings we have historically, but not new ones. So, for example 3.10 would error as a float, with a very good error message -- but I'm not 100% sure that's correct. It will be really annoying to users, since we know what they want, and could trivially convert it.

I'm torn on the correct path forward here, but I'm definitely convinced we should move to strings everywhere, and the question is how to handle the immediate 3.10 support and backwards compat. If we have a strong need to ship this code this week, I'm OK with special-casing '3.10' for now since we can make it less strict over time, but not more. We should pretty quickly follow up with a larger "strings everywhere" fix and docs update though I think.

[ericholscher]

Thinking more on this, I think we need to convert 3.10 to the proper value instead of breaking. Imagine a user upgrading from 3.9 to 3.10 -- if they just changed their version in the config, it would blow up -- this seems like a terrible UX to have the same field map to various data types depending on value. We should at least support strings for all versions, if we're going to make this upgrade.

[stsewd]

I have opened #8372 changing everything to strings (but numbers still work), and with a special case for 3.10 (number)

[astrojuanlu]

I confirm that this addresses the issue discussed in numba/numba#7182 without the need to switch to mamba ✔️ I opened #8375 with a couple of ideas for logging improvements.