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

path_rel2abs breaks python venv #64

Closed
DE0CH opened this issue Dec 30, 2023 · 11 comments
Closed

path_rel2abs breaks python venv #64

DE0CH opened this issue Dec 30, 2023 · 11 comments
Labels
bug Something isn't working

Comments

@DE0CH
Copy link
Contributor

DE0CH commented Dec 30, 2023

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 MLopez-Ibanez added the bug Something isn't working label Jan 4, 2024
@MLopez-Ibanez
Copy link
Owner

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
Copy link
Contributor Author

DE0CH commented Jan 6, 2024

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
Copy link
Owner

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
Copy link
Owner

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

@DE0CH
Copy link
Contributor Author

DE0CH commented Jan 7, 2024

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 r-lib/fs#440.

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

@MLopez-Ibanez
Copy link
Owner

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.

@MLopez-Ibanez MLopez-Ibanez reopened this Jan 7, 2024
@DE0CH
Copy link
Contributor Author

DE0CH commented Jan 7, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

DE0CH commented Jan 8, 2024

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
Copy link
Owner

Could you try with the latest revision?

@DE0CH
Copy link
Contributor Author

DE0CH commented Jan 10, 2024

Yep, the latest version works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants