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

Consider removing absolute paths from model.sh #124

Open
chimaerase opened this issue Jan 18, 2024 · 4 comments
Open

Consider removing absolute paths from model.sh #124

chimaerase opened this issue Jan 18, 2024 · 4 comments

Comments

@chimaerase
Copy link

As part of testing exported models, I realized that model.sh files generated during export contain several absolute paths to the export directory. Ideally from my perspective, .sh files should avoid including absolute paths, e.g. so the entire export directory can be arbitrarily relocated (less any external path dependencies in the model).

A simplified export script might be something like the following, where the working directory would be controlled by the user / client code:

#!/bin/bash
./model.bin >> out 2>> err
if [ $? -eq 0 ]; then
  echo success > finished
else
  echo failure > finished
fi
@chimaerase chimaerase changed the title Consider removing absolute paths from model.sh Consider removing absolute paths from model.sh Jan 18, 2024
@frothga
Copy link
Collaborator

frothga commented Jan 19, 2024

The original motivation for the absolute paths was to make it easier to launch a detached process from Java. I'll check if there is a good way to do this without relying on the shell script to change directories.

@frothga
Copy link
Collaborator

frothga commented Jan 19, 2024

Turns out that for remote jobs, it gets more complicated to handle paths when they're relative. Since this is not a high-priority issue, I am closing this as "won't fix".

@frothga frothga closed this as not planned Won't fix, can't repro, duplicate, stale Jan 19, 2024
@chimaerase
Copy link
Author

Thank you for taking a look! I agree this isn't a priority since the solution is complicated.

@frothga frothga reopened this Jan 22, 2024
@frothga
Copy link
Collaborator

frothga commented Jan 22, 2024

While looking at this issue, a thought came to me on how to deal with the remote path issue. I'm reopening this, just to keep it on the TODO list.

The main issue with relative remote paths is how to transmit them through SSH. The current procedure is to convert them to absolute paths, then quote if needed. The part about converting to absolute path could be removed, as there isn't really a good way to do this. The current method is just a hack that is rarely actually used, and doesn't really work when it is used. Instead, paths will be transmitted exactly as is.

The remaining issue is that a relative path to an executable will generally fail. It is usually an immediate reference to an executable in the current working directory (CWD), but "." isn't necessarily in the search path. Rather than requiring the user to configure this, we should prepend a dot to the path. However, we shouldn't do that with every single relative path. The trick here is to extend Host.quote() to take a flag indicating whether to treat the path as an executable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants