MLopez-Ibanez / irace

Iterated Racing for Automatic Algorithm Configuration
https://mlopez-ibanez.github.io/irace/
GNU General Public License v2.0
58 stars 14 forks source link

path_rel2abs breaks python venv #64

Closed DE0CH closed 9 months ago

DE0CH commented 10 months ago

My target runner is a python script, after I activate a virtual environment, I call irace with

$ irace --target-runner-launcher python3 --target-tunner target-irace.py --parameter-file parameters.txt --max-experiments 96 --seed 123
...
== irace == The call to targetRunner was:
/opt/homebrew/Cellar/python@3.11/3.11.6_1/Frameworks/Python.framework/Versions/3.11/bin/python3.11 '/Users/deyaochen/cs/irace-tuning3/target-runner.py' 1 1 819121060 /Users/deyaochen/cs/irace-tuning3/Instances/cplex_regions200-1.toml  --capping 1 --bound-max 5
...

This is incorrect because it cause the python script to be unable to find the installed modules in the virtual environment. I expect irace to call targetRunner with

/Users/deyaochen/cs/irace-tuning3/venv/bin/python3  '/Users/deyaochen/cs/irace-tuning3/target-runner.py' 1 1 819121060 /Users/deyaochen/cs/irace-tuning3/Instances/cplex_regions200-1.toml  --capping 1 --bound-max 5

For context,

$ which python3
/Users/deyaochen/cs/irace-tuning3/venv/bin/python3
$ ls -lah /Users/deyaochen/cs/irace-tuning3/venv/bin/python3
lrwxr-xr-x  1 deyaochen  staff    10B 22 Dec 07:54 /Users/deyaochen/cs/irace-tuning3/venv/bin/python3 -> python3.11

I suspect this is because path_real2labs follows symlinks. I don't think it should because there is information in a symlink. In this case, it tells python to look for the installed python modules in the virtual environment.

MLopez-Ibanez commented 10 months ago

We use normalizePath from base R and that follows symlinks. There is a normalize_path in the package xfun but it seems it only works when the symlink is the last component of the path?

Maybe we can replace this function completely with a combination of fs::path_abs() and fs::path_expand(). It would have to handle using Sys.which() the case when the path passed is to an executable that is in the PATH.

DE0CH commented 10 months ago

I think this approach can work. Though, what's the reason for converting it to an absolute path? Because I think the simplest is to just not touch the --target-runner-launcher argument and just call it as it (such as with system2).

MLopez-Ibanez commented 10 months ago

I think this approach can work. Though, what's the reason for converting it to an absolute path? Because I think the simplest is to just not touch the --target-runner-launcher argument and just call it as it (such as with system2).

The main reason is informational to help debugging problems and for reproducibility. Before we converted the paths to absolute, it was not uncommon that users will get their relative paths messed up. Also it is not uncommon to have "tune-v1/target-runner" and "tuner-v2/target-runner", but if you provide "./target-runner", then you cannot know what you ran just looking at the irace.Rdata file.

MLopez-Ibanez commented 10 months ago

Could you test the last commit? I added a fix that I hope will solve this.

DE0CH commented 9 months ago

I think the logic works, but it didn't work in practice. It hangs, probably due to a bug in fs, which I reported here https://github.com/r-lib/fs/issues/440.

For now, is there a way fs::is_file can be avoided? Shouldn't just fs::file_access be sufficient?

MLopez-Ibanez commented 9 months ago

Does the bug trigger with fs::is_dir() ? I should actually split this function into two, one for executable files than handles finding executables in the PATH and another for everything else that doesn't try to be that smart.

DE0CH commented 9 months ago

The bug doesn't trigger with fs::is_dir().

If you split the function into two, would you consider just leaving --target-runner-launcher as is? system2 would do the right thing. And if the users mess up their relative paths for this, it can only happen before irace is called so converting it to an absolute path won't really help.

MLopez-Ibanez commented 9 months ago

OK, try with the latest version.

The problem is when the call by system2() does not work or is calling something you did not expect and you do not know why. For example, you call --target-runner-launcher python and this ends up calling python3.6 instead of python3.7 and you do not know why. If the error prints /Users/deyaochen/cs/Anaconda/bin/python3.6 but you were expecting to call /Users/deyaochen/cs/irace-tuning3/venv/bin/python3 then it will be easy to see that Anaconda was found in the PATH before the other python installation. This is not theoretical, it has happened to users with multiple python and R installations. On the other hand, not following symlinks is not problematic since the target of the symlink is unambiguous. Thanks for reporting that!

Also, users will be surprised if --target-runner my-runner.sh does not work, but unless '.' is in the PATH, system2('my-runner.sh') does not work. Converting it to an absolute path will always work.

Also, I do not really trust system2() to do the right thing, since it invokes a shell, which may have its own way of setting the PATH variable (some of them shell specific) so figuring out which executable was actually called may be hard. The documentation of system2() refers to system() for the details on how executables are found and that documentation says: "There are many pitfalls in using ‘system’ to ascertain if a command can be run - ‘Sys.which’ is more suitable."

Also, it saves some CPU time to search the executable in PATH once and save its absolute path rather than let system2() do it for every call (there may be hundreds of thousands of calls in a single run of irace). It is perhaps a tiny amount of CPU time in most scenarios but it is easy to do.

DE0CH commented 9 months ago

I see. Thanks for the explanation. That makes sense.

Sorry, the latest version still doesn't work. I got confused and I meant that fs::is_dir('opt/homebrew/bin') doesn't hang, but fs::is_dir('opt/homebrew/bin/python3') still hangs. Sorry for the misunderstanding.

MLopez-Ibanez commented 9 months ago

Could you try with the latest revision?

DE0CH commented 9 months ago

Yep, the latest version works.