allegroai / clearml-agent

ClearML Agent - ML-Ops made easy. ML-Ops scheduler & orchestration solution
https://clear.ml/docs/
Apache License 2.0
235 stars 90 forks source link

Use correct Python version in Poetry init #179

Closed nfzd closed 6 months ago

nfzd commented 9 months ago

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:

jkhenning commented 8 months ago

Hi @nfzd,

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

nfzd commented 8 months ago

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).

jkhenning commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

nfzd commented 8 months ago

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 commented 7 months ago

@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 commented 7 months ago

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

nfzd commented 6 months ago

Updated to fix the merge conflict.