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 4 times, most recently from ce28d88 to dedc203 Compare February 3, 2025 16:33
Copy link
Collaborator

@cjllanwarne cjllanwarne left a comment

Choose a reason for hiding this comment

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

This seems well implemented and the test cases give me good confidence. My only minor concern is relying on a trailing '/' to decide whether something is a directory or not. If you can make me feel better about that, I'll happily 👍 . Oh, and I think a minor documentation improvement is possible too 😄

hail/python/hailtop/hailctl/batch/cli.py Outdated Show resolved Hide resolved


def real_absolute_cwd() -> str:
return real_absolute_expanded_path(os.getcwd())[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be adding a / to the end here to make cwd look like a directory?

Copy link
Member Author

@ehigham ehigham Feb 5, 2025

Choose a reason for hiding this comment

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

As you can tell, this sort of thing is unreliable and inconsistently applied. The short answer is no. I'm gonna change this implementation to use pathlib for local files.



def real_absolute_expanded_path(path: str) -> Tuple[str, bool]:
had_trailing_slash = path[-1] == '/' # NB: realpath removes trailing slash
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really the best we can do? Can we not make an os call to find out for sure whether or not it's a directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to take a detailed look at this PR now. I initially just assumed it was good to go but it's clear that's not possible.

Copy link
Member Author

ehigham commented Feb 5, 2025

This was mostly implemented by @danking - I just tweaked the tests to not drool invalid current working directories.
I hate trialing slashes to denote directories. If we're doing that anywhere here I'll gladly kill it.

@ehigham ehigham force-pushed the ehigham/hailctl-batch-submit-fixes branch 6 times, most recently from 04e7718 to 918f509 Compare February 7, 2025 16:53
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 918f509 to a42df02 Compare February 7, 2025 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants