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

Use correct Python version in Poetry init #179

Merged
merged 3 commits into from Mar 11, 2024

Conversation

nfzd
Copy link
Contributor

@nfzd nfzd commented Jan 5, 2024

It seems that the Poetry initialization always uses the system's default Python version, and not the one requested for the task.

This causes issues because it results in packages being installed with the wrong version of Pip (when the main script execution starts, poetry is run with the correct Python version, i.e., the requested one, and it crashes).

With this patch, it works:

  • Target Python version (mine: 3.7) installed on local machine and worker (both have system default of 3.10)
  • Local machine: using an environment with Poetry installed in Python 3.7
  • In pyproject.toml: python = "^3.7.0"
  • Locally: poetry env use 3.7 and poetry install, then add poetry.lock to VCS
  • poetry run python main_which_will_exec_remotely.py

@jkhenning
Copy link
Member

Hi @nfzd,

I'm not sure I understand the fix - the self._python variable is initialized at the start of the class

@nfzd
Copy link
Contributor Author

nfzd commented Jan 11, 2024

Hi @jkhenning,

true. But PoetryConfig is initialized when the Worker is set up (here) but the correct Python version is only determined much later within Worker.execute (within here, namely, here).

An alternative would be to set up the PoetryConfig afterwards, but I'm not sure that would be cleaner.

Perhaps it would make sense to remove the interpreter arg from PoetryConfig.__init__ to make clear that the Python binary will be read from the session (config).

@nfzd nfzd force-pushed the fix_poetry_init_python_ver branch 2 times, most recently from 45202c0 to 522fb07 Compare January 11, 2024 08:28
@jkhenning
Copy link
Member

Python version is only determined much later within Worker.execute (within here, namely, here).

The thing is that when resolving the version there, the code actually verifies this is a viable executable. In your change, you simply take the value from the task, and in case this is not a python version that's available in the execution environment, I assume we'll end up with a failure...

@nfzd
Copy link
Contributor Author

nfzd commented Jan 11, 2024

Looks to me like that check is done before the values are written to session._config, so that should always contain reasonable values? And that is where I read the values from.

@jkhenning
Copy link
Member

I think you're right, however that won't handle cases where the executable is overridden, such as here

@nfzd
Copy link
Contributor Author

nfzd commented Jan 11, 2024

Right. This case is currently also not handled (but I'm not sure whether the default of sys.executable used in PoetryApi will actually be different from the overwritten value?). One could either leave it this way, or assign the values to the config in any case -- having a single location from which to read the Python bin to use would be conceptually nice -- but I don't know whether that can cause issues elsewhere.

@jkhenning
Copy link
Member

@nfzd , apologies for the late reply - can't we just handle the override case as wll by updating the executable in the poetry class?

@nfzd
Copy link
Contributor Author

nfzd commented Feb 21, 2024

@jkhenning Take a look, now it should use the value from the override (if configured) also for Poetry.

@nfzd nfzd force-pushed the fix_poetry_init_python_ver branch from 0592d36 to 4ce3fed Compare March 6, 2024 07:41
@nfzd
Copy link
Contributor Author

nfzd commented Mar 6, 2024

Updated to fix the merge conflict.

@jkhenning jkhenning merged commit 2de1c92 into allegroai:master Mar 11, 2024
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

2 participants