Fix download_via_gsutil and rsync workdir commands

[gmittal]

rsync needs a trailing slash to have the intended behavior the following commands are expecting. Also the target_dir needs to be created before we copy stuff to it with gsutil.

[concretevitamin]

Nice! Just a question. OK either way as long as we document the behavior in comment somewhere.

[concretevitamin]

Assume this change isn't made. If my local workdir has /path/a/b,

• If we specify workdir as /path, then what do we see on remote? Expected /tmp/workdir/a/b.
• If we specify workdir as /path/, then what do we see on remote? Expected, arguably, /tmp/workdir/a/b.

How would adding this change affect things?

[gmittal]

If we do ~/Downloads/tpu, then /tmp/workdir/tpu. But the downstream training command that gets launched does cd /tmp/workdir && python -u models/... (which won't work since models is actually tpu/models).

It seems reasonable that remote workdir tree should be identical to the local workdir tree? Instead of having an additional layer with the name of the local working directory (tpu).

[michaelzhiluo]

I agree with Zongheng, doesn't change anything. If you do ray attach config.yml and go to /tmp/workdir, models will be there, not tpu.

[michaelzhiluo]

Other wise, LGTM

[gmittal]

Hmmm... that is strange because it was failing for me with my fresh AWS setup without the trailing slash.

[concretevitamin]

Maybe it's ray versions having different behavior? I'm using 1.5.2. Regardless if this is a confirmed issue, we should fix!

…

On Fri, Oct 29, 2021 at 12:16 AM Gautam Mittal ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In prototype/sky/execution.py <#5 (comment)> : > @@ -206,7 +206,7 @@ def execute(dag: sky.Dag, dryrun: bool = False, teardown: bool = False): if task.workdir is not None: runner.add_step( 'sync', 'Sync files', - f'ray rsync_up {cluster_config_file} {task.workdir} {SKY_REMOTE_WORKDIR}' + f'ray rsync_up {cluster_config_file} {task.workdir}/ {SKY_REMOTE_WORKDIR}' Hmmm... that is strange because it was failing for me with my fresh AWS setup without the trailing slash. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#5 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEQWHXWF4UASSI4WZU43ETUJJC5TANCNFSM5G6RNYAQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[gmittal]

I'm on ray==1.7.0. I think we if we can agree on whether we want the the remote workdir tree to be the same as the local workdir then we can keep the fix as is in this PR.