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

[Draft] Detect Python version using Poetry #30

Closed
wants to merge 1 commit into from

Conversation

frafra
Copy link

@frafra frafra commented Feb 17, 2021

It is useful to use prefixes like ^ and ~ when requiring a Python version, without picking a specific minor release. As Poetry needs to be installed and fully working, why don't rely on it to detect which Python version satisfies such requirement automatically?

@ulgens
Copy link
Contributor

ulgens commented Feb 17, 2021

I'm not sure poetry run can be called there. This buildpack focuses on converting pyproject.toml to requirements.txt, not installing the environment itself.

@ulgens
Copy link
Contributor

ulgens commented Feb 17, 2021

@zyv Any idea why CI pipeline is not triggered?

@frafra
Copy link
Author

frafra commented Feb 17, 2021

I'm not sure poetry run can be called there. This buildpack focuses on converting pyproject.toml to requirements.txt, not installing the environment itself.

I understand, but this PR does not install the environment more than running poetry export (it creates an environment, but it does not install any package):

poetry export -f "$REQUIREMENTS_FILE" "${EXPORT_PARAMETERS[@]}" > "$REQUIREMENTS_FILE.orig"

@frafra frafra changed the title Detect Python version using Poetry [Draft] Detect Python version using Poetry Feb 17, 2021
@frafra
Copy link
Author

frafra commented Feb 17, 2021

It looks like it is pretty useless, as Poetry does not seem to enforce the Python version, so the result would be similar to just executing the same command without poetry run.

@stohrendorf stohrendorf marked this pull request as draft February 18, 2021 06:42
@zyv
Copy link
Contributor

zyv commented Feb 18, 2021

It is useful to use prefixes like ^ and ~ when requiring a Python version, without picking a specific minor release.

Could you please explain what is the problem that you are trying to solve here in the first place?

Heroku supports following scenarios:

  1. You can specify a concrete runtime version using runtime.txt
  2. You can avoid specifying runtime, in which case the current default will be used

This buildpack doesn't support (2), because it enables random version drift between development and production. Even if your runtime and Heroku runtime match today, tomorrow when Heroku will change the default runtime, redeploying exactly the same code will cause a completely different state to be run - and no-one will notice.

In as far as the first option is concerned, we support both possibilities:

  1. You go the Heroku way, and specify your runtime using runtime.txt - in this case, you need to tell the buildpack to accept it - see the documentation here: https://github.com/moneymeets/python-poetry-buildpack#runtimetxt . We must do it this way, otherwise there will be an inconsistency between your development and production, and it will never be noticed.
  2. You go the full Poetry way, but then you need to specify a concrete version including the minor in pyproject.toml, because Poetry doesn't lock minors for runtime versions - the support for runtimes works differently as it does for normal packages!

Having that said, it is not clear to us what scenario you want to enable with your patch, the implementation notwithstanding.

@zyv
Copy link
Contributor

zyv commented Feb 18, 2021

@zyv Any idea why CI pipeline is not triggered?

No clue, maybe because this is a draft PR, and GitHub doesn't trigger the Actions in this case?

@stohrendorf
Copy link
Contributor

No clue, maybe because this is a draft PR, and GitHub doesn't trigger the Actions in this case?

No, it was a regular PR before I changed it to "draft" because of "draft" in the title.

@frafra
Copy link
Author

frafra commented Feb 18, 2021

@zyv Sure, sorry for not having explained in the first place.

Fixing the Python interpreter to a specific version mean having many problems when collaborating with other developers or when a system upgrade happens. Sticking to version ~=3.9 or similar is pretty reasonable, as minor Python releases do not break compatibility. Another benefit is to be able to get a security patched release for free.

Adding a runtime.txt file would be great, so I could have a more relaxed requirement for Poetry and a stricter one in production, but this is not possible at the moment:

if [ -f "$RUNTIME_FILE" ] ; then
log "$RUNTIME_FILE found, delete this file from your repository!" >&2
exit 1
fi

Poetry does not lock the interpreter version either.

@zyv
Copy link
Contributor

zyv commented Feb 18, 2021

Adding a runtime.txt file would be great, so I could have a more relaxed requirement for Poetry and a stricter one in production, but this is not possible at the moment:

It is possible, it seems that your reading of the code is wrong - please see again my explanation and linked documentation.

@zyv
Copy link
Contributor

zyv commented Feb 18, 2021

Another benefit is to be able to get a security patched release for free.

It is not possible to get security patched release "for free" - Heroku doesn't change the runtime of an already deployed application, even upon restarts. You will need to release and re-deploy the application manually after runtime security update has been deployed by Heroku in any case - whether you fix the minor or not doesn't make any difference.

@zyv
Copy link
Contributor

zyv commented Feb 27, 2021

The approach in the pull request is misguided in that at the time when compile is run the target Python interpreter is not available and so the code doesn't make any sense. The system is only guaranteed to have Python interpreter installed which is specific to the selected stack:

One could imagine using the system Python and packaging (https://packaging.pypa.io/en/latest/index.html) to parse versions & version specifiers to compare them against currently supported (and/or all available) runtimes on Heroku.

Now that it seems to be a recurring topic, I went ahead and opened an issue asking whether there such an API is available, but failing that the only way to know would be to parse HTML or Markdown files: heroku/heroku-buildpack-python#1181 . The parsing approach has so far been rejected, because it's just way too fragile, complex and laborious.

We still support two escape hatches, however, as explained in the documentation: DISABLE_POETRY_CREATE_RUNTIME_FILE and PYTHON_RUNTIME_VERSION. Since no explanation has been provided what exactly isn't working, I'm rejecting this PR.

See also #6 and #10 for related discussion.

@zyv zyv closed this Feb 27, 2021
@zyv
Copy link
Contributor

zyv commented Feb 27, 2021

No clue, maybe because this is a draft PR, and GitHub doesn't trigger the Actions in this case?

No, it was a regular PR before I changed it to "draft" because of "draft" in the title.

@ulgens @stohrendorf thanks for the notice, it seems that a trigger was missing to run CI also on PRs - I've tried to adjust it so that the CI also runs on (external) PRs in addition to pushes and internal PRs, but still runs only once and not multiple times.

@zyv
Copy link
Contributor

zyv commented Sep 22, 2022

Meanwhile Heroku buildpack added support for querying the latest supported Python version by importing a file with specially defined shell variables:

heroku/heroku-buildpack-python#1181 (comment)

https://raw.githubusercontent.com/heroku/heroku-buildpack-python/main/bin/default_pythons

So we could in theory write the latest minor version per major version into runtime.txt if requested with something like ^3.9. We are not planning to do so, because it’s a lot of work for us and we already provide three options:

  • Specify exact minor in Poetry
  • Specify runtime per env variable
  • Use normal runtime.txt

… but at least, it’s a possibility now.

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

Successfully merging this pull request may close these issues.

4 participants