pulp / oci_env

9 stars 33 forks source link

Use module name (-m) to run pytest and pip #148

Closed dosas closed 7 months ago

dosas commented 8 months ago

when using python version other than 3.8 pip and pytest were not using the correct python version

mdellweg commented 8 months ago

And how does python3 know the right one to choose? I believe with multiple versions of python installed in one container there is something going wrong here.

dosas commented 8 months ago

The switch python script takes care of that, it updates the alternative.

It is no problem to have multiple python versions as long as python -m is used.

mdellweg commented 8 months ago

It is no problem to have multiple python versions as long as python -m is used.

I can see how it works, but the result looks more like a space station to me than a container.

dosas commented 8 months ago

This PR is not about changing the architecture or workflow of oci-env. It tries to provide a fix for an 'advertised' functionality, the possibility to use a different python version than 3.8. This functionality was broken an unnoticed.

If you have general issues with this workflow or architecture this is out of scope for this PR. You are welcome to provide an alternative solution.

mdellweg commented 8 months ago

I guess switch python is broken then. This line is supposed to do it. https://github.com/pulp/oci_env/blob/3fd2c2486fe9f95ebcf841d6fb3fde24c345d83b/base/switch_python#L17

dosas commented 8 months ago

No, I disagree. The only way to have the correct python version for pip and pytest is using -m flag.

mdellweg commented 8 months ago

I see it adding the alternative for pip3. Looks like it's missing to put one for pip too.

dosas commented 8 months ago

So we have two possibilities:

  1. Run pip and pytest and hope they chose the correct python version or
  2. Run the correct python version and let it invoke pip and pytest

I do not see any point in favor the first one.

If it works we can additionally add pip to switch_python or change every occurrence of pip to pip3.

mdellweg commented 8 months ago

It is ridiculous in the first place to have multiple versions of python in one container fighting over precedence. If you can find a way to fix up switch python, that might be an acceptible stop gap, but in the long run, when you want another python, start with a different base image. When you do this, think about a user calling oci_env shell. They don't understand that pip or pytest is the wrong command to use.

dosas commented 8 months ago

If it is 'ridiculous' to have multiple versions of python, the logical option would be to remove the broken switch_python script.

dkliban commented 8 months ago

@jctanner are you using the feature that lets you switch python versions? It doesn't seem to be working correctly. We are considering removing it all together.

jctanner commented 8 months ago

We use it all the time with no issue https://github.com/ansible/galaxy_ng/blob/master/profiles/base/Dockerfile

dosas commented 8 months ago

We use it all the time with no issue https://github.com/ansible/galaxy_ng/blob/master/profiles/base/Dockerfile

That is because of the -m option :D

Are you invoking tests (unit/functional) with it?

jctanner commented 8 months ago

@dosas no we aren't using it for functional or unit testing. The functional and unit tests run in CI based on the ansible playbooks created by the pulp plugin template. It builds an image and runs a container via ansible to do all that work, including a matrix with s3/azure/fs backing filestores. The galaxy team does not tend to write functional tests in the galaxy_ng repo. Instead, in our CI we also have integration tests (not pulp native testing) that do use the oci profiles+image.

cutwater commented 7 months ago

@dosas @mdellweg I wonder why can't oci-env just use venv created by required version of python. This will effectively fix all lookup issues and will not require tampering with system level python?

mdellweg commented 7 months ago

We provide different base images with different python versions for this. Closing now.

@dosas @mdellweg I wonder why can't oci-env just use venv created by required version of python. This will effectively fix all lookup issues and will not require tampering with system level python?

This would still need multiple python versions to coexist in the same container.

dosas commented 7 months ago

For reference https://github.com/pulp/pulp-oci-images/issues/586