[query] Avoid Class A Operations. #13885

danking · 2023-10-23T14:46:11Z

CHANGELOG: Hail Query-on-Batch previously used Class A Operations for all interaction with blobs. This change ensures that QoB only uses Class A Operations when necessary.

Inspired by @jigold 's file system improvement campaign, I pursued the avoidance of "list" operations. I anticipate this reduces flakiness in Azure (which is tracked in #13351) and cost in Azure.

I enforced aiotools.fs terminology on hail.fs and Scala:

FileStatus. Metadata about a blob or file. It does not know if a directory exists at this path.
FileListEntry. Metadata from a list operation. It knows if a directory exists at this path.

Variable names were updated to reflect this distinction:

fileStatus / fileStatuses
fle/ fles / fileListEntry / fileListEntries, respectively.

listStatus renamed to listDirectory for clarity.

In both Azure and Google, fileStatus does not use a list operation.

fileListEntry can be used when we must know if a directory exists. I just rewrote this from first principles because:

In neither Google nor Azure did it check if the path was a directory and a file.
In Google, if the directory entry wasn't in the first page, it would fail (NB: there are fifteen non-control characters in ASCII before /, if the page size is 15 or fewer, we'd miss the first entry with a / at the end).
In Azure, we issued both a get and a list.

There are now unit tests for this method.

copyMerge and concatenateFiles previously used O(N_FILES) list operations, they now use O(N_FILES) get operations.
Writers that used exists to check for a _SUCCESS file now use a get operation.
Index readers, import BGEN, and import plink all now check file size with a get operation.

That said, overall, the bulk of our Class A Operations are probably writes.

@jigold

CHANGELOG: Hail Query-on-Batch previously used Class A Operations for all interaction with blobs. This change ensures that QoB only uses Class A Operations when necessary. Inspired by @jigold 's file system improvement campaign, I pursued the avoidance of "list" operations. I anticipate this reduces flakiness in Azure (which is tracked in hail-is#13351) and cost in Azure. I enforced aiotools.fs terminology on hail.fs and Scala: 1. `FileStatus`. Metadata about a blob or file. It does not know if a directory exists at this path. 2. `FileListEntry`. Metadata from a list operation. It knows if a directory exists at this path. Variable names were updated to reflect this distinction: 1. `fileStatus` / `fileStatuses` 2. `fle`/ `fles` / `fileListEntry` / `fileListEntries`, respectively. `listStatus` renamed to `listDirectory` for clarity. In both Azure and Google, `fileStatus` does not use a list operation. `fileListEntry` can be used when we must know if a directory exists. I just rewrote this from first principles because: 1. In neither Google nor Azure did it check if the path was a directory and a file. 2. In Google, if the directory entry wasn't in the first page, it would fail (NB: there are fifteen non-control characters in ASCII before `/`, if the page size is 15 or fewer, we'd miss the first entry with a `/` at the end). 3. In Azure, we issued both a get and a list. There are now unit tests for this method. --- 1. `copyMerge` and `concatenateFiles` previously used `O(N_FILES)` list operations, they now use `O(N_FILES)` get operations. 2. Writers that used `exists` to check for a _SUCCESS file now use a get operation. 3. Index readers, import BGEN, and import plink all now check file size with a get operation. That said, overall, the bulk of our Class A Operations are probably writes.

CHANGELOG: Introduce `hl.fs.fast_stat` and `hl.hadoop_fast_stat` which use cheaper Class B Operations in Google Cloud Storage rather than Class A Operations. Users of `hl.hadoop_stat` and `hl.fs.stat` should consider switching. This PR extends hail-is#13885 into the public API.

This reverts commit ab91750.

danking · 2023-12-05T02:12:01Z

OK, I think this is finally ready for review. It can wait until January if necessary.

jigold

This is an awesome change!

jigold · 2023-12-06T15:24:40Z

hail/src/main/scala/is/hail/expr/ir/GenericLines.scala

@@ -252,7 +252,7 @@ object GenericLines {

  def read(
    fs: FS,
-    fileListEntries0: IndexedSeq[FileListEntry],
+    fileListEntries0: IndexedSeq[_ <: FileStatus],


Change the variable name here to fileStatuses0?

hail/src/main/scala/is/hail/io/bgen/LoadBgen.scala

hail/python/test/hail/utils/test_utils.py

jigold · 2023-12-06T19:24:12Z

hail/src/main/scala/is/hail/io/fs/FS.scala

+        dirFle = fle
+      }
+
+      continue = it.hasNext && (fle.getActualUrl <= withSlash)


Can you put a comment here on why you can check for fle.getActualUrl <= withSlash?

jigold · 2023-12-06T19:28:53Z

hail/src/main/scala/is/hail/io/fs/GoogleStorageFS.scala

    new BlobStorageFileListEntry(
-      s"gs://${ blob.getBucket }/$name",
+      s"gs://${ blob.getBucket }/${blob.getName}",


Suggested change

s"gs://${ blob.getBucket }/${blob.getName}",

s"gs://${ blob.getBucket }/${ blob.getName }",

I've made this change.

We do use ${url} pervasively. Should that be ${ url }?

jigold · 2023-12-06T19:30:38Z

hail/src/main/scala/is/hail/io/fs/GoogleStorageFS.scala

    if (url.path == "")
-      return new BlobStorageFileListEntry(s"gs://${url.bucket}", null, 0, true)
+      return GoogleStorageFileListEntry.dir(url)


Why does the return type here match up between GoogleStorageFileListEntry.dir and FileStatus?

A FileListEntry is a subtype of a FileStatus.

A FileStatus is a thing about which we know the name, the size, the creation time, etc. A FileListEntry is something we know all of that and also we know for sure if there is a directory with this name.

jigold · 2023-12-06T19:32:19Z

hail/src/main/scala/is/hail/io/fs/HadoopFS.scala

@@ -171,6 +174,14 @@ class HadoopFS(private[this] var conf: SerializableHadoopConfiguration) extends
    files.map(fileListEntry => new HadoopFileListEntry(fileListEntry))
  }

+  override def fileStatus(url: URL): FileStatus = {


return types don't match?

See above, FileListEntry is a sub-type of FileStatus. The compiler wouldn't compile this code otherwise.

x

danking · 2023-12-07T22:38:12Z

Thanks for the review! I think I've addressed everything in this commit: 1bac0f1

jigold · 2024-01-03T18:47:11Z

hail/src/main/scala/is/hail/expr/ir/GenericLines.scala

    nPartitions: Option[Int],
    blockSizeInMB: Option[Int],
    minPartitions: Option[Int],
    gzAsBGZ: Boolean,
    allowSerialRead: Boolean,
    filePerPartition: Boolean = false
  ): GenericLines = {
-    val fileListEntries = fileListEntries0.zipWithIndex.filter(_._1.getLen > 0)
+    val fileListEntries = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)


Should this variable be named fileStatuses now instead of fileListEntries?

jigold · 2024-01-05T19:43:49Z

hail/src/main/scala/is/hail/expr/ir/AbstractMatrixTableSpec.scala

+      using(fs.open(metadataFile)) { in => parse(in) }
+    } catch {
+      case exc: FileNotFoundException =>
+        if (fs.isFile(path)) {


This seems a bit weird to me that we're catching a FileNotFoundException and then testing whether it's a file or a directory. Are we catching the wrong exception here?

Notice that we're catching FileNotFoundException for the metadataFile, but then checking if the path (i.e. the matrix table path) is a file or a directory.

jigold · 2024-01-05T20:05:47Z

hail/src/test/scala/is/hail/io/fs/FSSuite.scala

  @Test def testFileListEntryOnFile(): Unit = {
    // file
    val f = r("/a")
    val s = fs.fileListEntry(f)
    assert(s.getPath == f)
-    assert(s.isFile)
-    assert(!s.isDirectory)


Why did you get rid of these tests here? They seem valuable to me.

mistake, restored.

jigold · 2024-01-05T20:06:46Z

hail/src/test/scala/is/hail/io/fs/FSSuite.scala

+  @Test def testFileStatusOnDirIsFailure(): Unit = {
+    val f = r("/dir")
+    TestUtils.interceptException[FileNotFoundException](r("/dir"))(
+      fs.fileStatus(r("/dir"))


I thought you could create a fileStatus object for a directory. It just doesn't have the directory information as a FLE would have.

You can only create a fileStatus for a file/object not a directory/prefix. In particular, no single Class B Operation can reliably determine if a path is a directory. For example, if a GET for the object gs://bucket/dir/a returns 200, I know that gs://bucket/dir is a directory but if it returns 404, I don't know if that directory does or does not exist.

jigold · 2024-01-05T20:07:25Z

hail/src/test/scala/is/hail/io/fs/FSSuite.scala

  @Test def testFileListEntryOnDir(): Unit = {
    // file
    val f = r("/dir")
    val s = fs.fileListEntry(f)
    assert(s.getPath == f)
-    assert(!s.isFile)


Same comment about removing these tests and whether these are still valuable. Same below.

restored and restored in the next test.

xx

jigold · 2024-01-08T17:02:01Z

hail/src/main/scala/is/hail/expr/ir/GenericLines.scala

@@ -260,8 +260,8 @@ object GenericLines {
    allowSerialRead: Boolean,
    filePerPartition: Boolean = false
  ): GenericLines = {
-    val fileListEntries = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)
-    val totalSize = fileListEntries.map(_._1.getLen).sum
+    val fileSTatuses = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)


Suggested change

val fileSTatuses = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)

val fileStatuses = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)

jigold · 2024-01-08T17:02:11Z

hail/src/main/scala/is/hail/expr/ir/GenericLines.scala

-    val fileListEntries = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)
-    val totalSize = fileListEntries.map(_._1.getLen).sum
+    val fileSTatuses = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)
+    val totalSize = fileSTatuses.map(_._1.getLen).sum


Suggested change

val totalSize = fileSTatuses.map(_._1.getLen).sum

val totalSize = fileStatuses.map(_._1.getLen).sum

jigold · 2024-01-08T17:02:21Z

hail/src/main/scala/is/hail/expr/ir/GenericLines.scala

@@ -276,7 +276,7 @@ object GenericLines {
      case None =>
    }

-    val contexts = fileListEntries.flatMap { case (fileListEntry, fileNum) =>
+    val contexts = fileSTatuses.flatMap { case (fileListEntry, fileNum) =>


Suggested change

val contexts = fileSTatuses.flatMap { case (fileListEntry, fileNum) =>

val contexts = fileStatuses.flatMap { case (fileListEntry, fileNum) =>

casing fixed

danking mentioned this pull request Oct 23, 2023

[fs] expose hl.fast_stat and hl.hadoop_fast_stat to users #13886

Closed

Dan King added 2 commits October 23, 2023 11:46

missing FakeFS things

aabb256

one more missing fake thing

1ab9693

Dan King added 3 commits October 23, 2023 12:29

fix error message and test for it

cae45a7

fixes for GCS

fdcaec0

fix

81fda0a

Dan King added 13 commits October 23, 2023 16:36

fix

6bd915a

fix gcs

66b45bf

use isFile as much as possible

d269505

fix

3b97286

fix

abb5129

fixes

7d069c5

match using DOTALL so that the . can match a newline

4402fb1

a few more fixes

48e6dab

fix lint

ab91750

Revert "fix lint"

9557c8a

This reverts commit ab91750.

finally get google working too

dcb6277

fix comilation error

67fbb80

test suite improvements

43b6b6f

danking mentioned this pull request Oct 30, 2023

[query] In Azure, QoB sees elevated rates of weird errors from the Azure Blob Storage SDK #13351

Closed

Dan King added 5 commits November 2, 2023 09:51

make FS serializable againt

54f9f27

fix tests

89f9e98

fix error

1b50751

ensure HadoopFSURL.toString is a qualified path

0765ad4

new error message

2afcbce

danking assigned jigold Dec 5, 2023

jigold previously requested changes Dec 6, 2023

View reviewed changes

comments

1bac0f1

variable rename was incomplete

e3ba50a

jigold previously requested changes Jan 5, 2024

View reviewed changes

address comments

8be3389

jigold previously requested changes Jan 8, 2024

View reviewed changes

danking force-pushed the reduce-class-a branch from 1303d8e to 8be3389 Compare January 9, 2024 15:06

Dan King added 4 commits January 9, 2024 10:33

Merge commit '5606d94cb1' into reduce-class-a

81e43df

scalafmt

b9e0bde

Merge remote-tracking branch 'hi/main' into reduce-class-a

a9d9933

case

f70c46d

jigold approved these changes Jan 9, 2024

View reviewed changes

danking merged commit 3ddbf5f into hail-is:main Jan 9, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Avoid Class A Operations. #13885

[query] Avoid Class A Operations. #13885

danking commented Oct 23, 2023

danking commented Dec 5, 2023

jigold left a comment

jigold Dec 6, 2023

jigold Dec 6, 2023

jigold Dec 6, 2023

danking Dec 7, 2023

jigold Dec 6, 2023

danking Dec 7, 2023

jigold Dec 6, 2023

danking Dec 7, 2023

danking commented Dec 7, 2023

jigold Jan 3, 2024

danking Jan 5, 2024

jigold Jan 5, 2024

danking Jan 5, 2024

jigold Jan 5, 2024

danking Jan 5, 2024

jigold Jan 5, 2024

danking Jan 5, 2024

jigold Jan 5, 2024

danking Jan 5, 2024

jigold Jan 8, 2024

jigold Jan 8, 2024

jigold Jan 8, 2024

	s"gs://${ blob.getBucket }/${blob.getName}",
	s"gs://${ blob.getBucket }/${ blob.getName }",

	val fileSTatuses = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)
	val fileStatuses = fileStatuses0.zipWithIndex.filter(_._1.getLen > 0)

	val totalSize = fileSTatuses.map(_._1.getLen).sum
	val totalSize = fileStatuses.map(_._1.getLen).sum

	val contexts = fileSTatuses.flatMap { case (fileListEntry, fileNum) =>
	val contexts = fileStatuses.flatMap { case (fileListEntry, fileNum) =>

[query] Avoid Class A Operations. #13885

[query] Avoid Class A Operations. #13885

Conversation

danking commented Oct 23, 2023

danking commented Dec 5, 2023

jigold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danking commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment