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

[hailctl] batch submit fixes #14805

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ehigham
Copy link
Member

@ehigham ehigham commented Jan 30, 2025

CHANGELOG: Fix many issues, including (hail#14274), with hailctl batch submit introduced in 0.2.127.
Fixes #14274, Replaces #14351 (authored by @jigold) and #14186 (authored by @danking).
This change has low security impact

Dan's original commit message is as follows:
"""
Issues resolved herein:

  1. build.yaml tests must not use exit 0 as it exits the test early.
  2. Always prefer orjson to json.
  3. Add --wait which waits for the submitted batch to complete and exits success only when the batch is success.
  4. Whenever working with paths, we must use the realpath which resolves symlinks. In particular, on Mac OS X, /tmp is a symlink to /private/tmp and Python's APIs are inconsistent on whether they return a realpath or a path with symlinks. [1]
  5. If the destination looks like a directory (e.g. "bar:/foo/", "bar:/"), the tests all suggest we should copy into not to. We now check for a trailing slash and copy into.
  6. ln -s src dst means different things depending on whether dst is an extant folder or not. In this PR, I prefer to always be fully explicit so I never rely on ln detecting the destination is a directory and acting differently. Put differently: file_input_to_src_dest now never returns a file source and a destination folder.
  7. We need to create the real_absolute_cwd() on the job before we cd into it.
  8. test_dir_outside_curdir suggests that --file foo/:/ is meant to copy the contents of foo into the root. This cannot be implemented with our symlink strategy (you can't replace the root with a symlink), so I changed the interpretation: a trailing slash on the source is meaningless. If the destination ends in a slash, we "copy into", otherwise we "copy to".
  9. Add examples of --files usage.

[1]:

In [1]: import tempfile
   ...: tempfile.TemporaryDirectory()
Out[1]: <TemporaryDirectory '/var/folders/x1/601098gx0v11qjx2l_7qfw2c0000gq/T/tmp_pmj3lr9'>

In [2]: import os
   ...: os.getcwd()
Out[2]: '/private/tmp'

In [3]: !ls -al /tmp
lrwxr-xr-x@ 1 root  wheel  11 Aug  2 05:44 /tmp -> private/tmp

"""

@ehigham
Copy link
Member Author

ehigham commented Jan 30, 2025

This change is @jigold's edits manually applied on main.
I couldn't figure out the merge conflicts when rebasing #14351.
Mixing merges and rebases causes havoc!

@ehigham ehigham force-pushed the ehigham/hailctl-batch-submit-fixes branch from d1c6edb to a2a376e Compare January 30, 2025 19:12
Copy link
Collaborator

@chrisvittal chrisvittal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks for pushing this over the line.

@ehigham ehigham force-pushed the ehigham/hailctl-batch-submit-fixes branch 2 times, most recently from 412702d to cc80326 Compare January 30, 2025 23:07
CHANGELOG: Fix many issues, including (hail#14274), with hailctl batch submit introduced in 0.2.127.
Fixes hail-is#14274, Replaces hail-is#14351 (authored by @jigold)
@ehigham ehigham force-pushed the ehigham/hailctl-batch-submit-fixes branch from cc80326 to ce28d88 Compare January 31, 2025 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants