Arvados

1. output everything in this dir
2. output everything in this dir that matches this glob
3. output everything in this dir that matches any of these globs
4. output everything in this dir that matches any of these globs, but not this glob
5. output everything in this dir that matches any of these globs, and apply this path translation

Ideally this can all be done inside the container instead, using the shell or some other programming language of your choice. You could also add a subsequent step to the workflow that rearranges/extracts the desired files (a useful pattern for other situations too, like improving container reuse in downstream work that doesn't need to see the entire output).

Tom Clegg wrote:

I'm not keen on this feature. It seems to creep in an awkward direction:

1. output everything in this dir
2. output everything in this dir that matches this glob
3. output everything in this dir that matches any of these globs
4. output everything in this dir that matches any of these globs, but not this glob
5. output everything in this dir that matches any of these globs, and apply this path translation

Nobody is asking for 4 and 5.

Ideally this can all be done inside the container instead, using the shell or some other programming language of your choice. You could also add a subsequent step to the workflow that rearranges/extracts the desired files (a useful pattern for other situations too, like improving container reuse in downstream work that doesn't need to see the entire output).

The client for this feature is arvados-cwl-runner (because output globs defined in the tool wrapper), not individual tools. The specific problem is intended to solve is programs that produce extra output that we don't want to upload, but gets uploaded anyway. The obvious solution is to have some way to specify what should and should not be uploaded.

I agree arvados-cwl-runner's needs are important but I would still prefer to find a way to accommodate them without feeding the "container request includes a mini-language for munging inputs and outputs in various ways" pattern.

I think this is fine if we can draw the line at "list of globs to include" (e.g., no "exclude" feature) and specify the globs we accept (e.g., gitignore style).

1. cr mounts an existing collection in read+write mode at "/mnt/foo"
2. cr output directory is "/mnt/foo" or /mnt/foo/bar"
3. cr output glob is "*.txt"
4. crunch-run removes files from the original collection

It would certainly be easier to write the code -- and possibly easier to understand/use? -- if "include dir1 and its contents" is spelled "dir1/**" rather than "dir1". There is a doublestar glob matching package that works for us out of the box if we do this.

Tom Clegg wrote in #note-31:

It would certainly be easier to write the code -- and possibly easier to understand/use? -- if "include dir1 and its contents" is spelled "dir1/**" rather than "dir1". There is a doublestar glob matching package that works for us out of the box if we do this.

The client doesn't know ahead of time if a pattern refers to a directory or not. The client of this is CWL which specifies POSIX glob matching which only has '?', character classes, and single star, not double star.

That said, since it provides multiple patterns and it only has to match any one pattern, we could double up every pattern as both "foo" and "foo/**", providing that patterns like "f*/**" do the right thing.

For reference this is how arvados-cwl-runner does it, the load bearing part is Python fnmatch() which breaks it into path segments and compares each segment against the glob pattern for that segment:

https://git.arvados.org/arvados.git/blob/HEAD:/sdk/cwl/arvados_cwl/fsaccess.py#l109

If Go doesn't have a convenient glob match function, I believe posix glob patterns can be fairly trivially converted into regular expressions.

12430-output-glob @ 45ff656e5af49b18845c443bc6957b283063070e -- developer-run-tests: #4121

12430-output-glob @ c2ab17464100b4914b65473175ff1c85b102f1e3 -- developer-run-tests: #4131

• All agreed upon points are implemented / addressed.
  • ** and [abc] and [^abc] also work (easier than not matching them -- is this ok?)
  • directory path itself does not match contents -- but ["directorypath", "directorypath/**"] does if that's what you want.
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • n/a
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • automated tests pass. no manual testing.
• Documentation has been updated.
  • ✅
• Behaves appropriately at the intended scale (describe intended scale).
  • adds per-file overhead, but I think it will be negligible compared to the work involved in copying a file, unless the number of globs given is unreasonable.
• Considered backwards and forwards compatibility issues between client and server.
  • existing container/container_request records are backfilled with output_glob=[] during migration, so container reuse will continue to match old containers with new CRs.
  • until all of client/rails/controller/a-d-c/crunch-run are upgraded, the old behavior (output all files) will continue.
• Follows our coding standards and GUI style guidelines.
  • ✅

I went with output_glob as written above, but I'm still wondering whether it should be output_globs, being an array.

The output_glob description is pretty vague. Just as a matter of adequate API documentation, our documentation should probably include a copy of the "Patterns" documentation from doublestar:

https://github.com/bmatcuk/doublestar?tab=readme-ov-file#patterns

On that note, that section says: A doublestar (**) should appear surrounded by path separators such as /**/. This makes wonder if matching patterns as either files or directories requires ["foo", "foo/*", "foo/**/*"] ? Perhaps a few more test cases would be beneficial?

I see TestApplyGlobsToCollectionFS has a trailing /** test but TestApplyGlobsToFilesAndDirs has fewer, different test cases, which don't include trailing /** ? Shouldn't these have the same test cases?

It would be helpful to have a comment explaining that "applyGlobsToFilesAndDirs" filters out files that were found on disk before they are uploaded, whereas "applyGlobsToCollectionFS" removes files from the output collection when it has been pre-populated.

In copier.walkMount:

    case !srcMount.Writable:
        cp.logger.Printf("copying %q from %v/%v", outputRelPath, srcMount.PortableDataHash, strings.TrimPrefix(srcRelPath, "./"))
        mft, err := cp.getManifest(srcMount.PortableDataHash)
        if err != nil {
            return err
        }
        cp.manifest += mft.Extract(srcRelPath, dest).Text

One of the performance issues we are trying to fix is a situation where the tool has staged 4000 files in 4000 different collections in the working directory. It turns out that even though it is just doing a manifest text copy, loading 4000 collections one by one, at 1 second each, still takes over an hour. This implementation of output_glob would end up doing all that work only to immediately throw it out.

    output_glob text DEFAULT '[]'::text

Is there a reason not to use ::json or ::jsonb here?

Peter Amstutz wrote in #note-38:

The output_glob description is pretty vague. Just as a matter of adequate API documentation, our documentation should probably include a copy of the "Patterns" documentation from doublestar:

https://github.com/bmatcuk/doublestar?tab=readme-ov-file#patterns

Added more detail & examples.

On that note, that section says: A doublestar (**) should appear surrounded by path separators such as /**/. This makes wonder if matching patterns as either files or directories requires ["foo", "foo/*", "foo/**/*"] ? Perhaps a few more test cases would be beneficial?

No, matching either a file or an entire subtree requires just ["foo", "foo/**"].

Added a bunch more test cases, including unit tests that more explicitly check file vs. directory handling.

I see TestApplyGlobsToCollectionFS has a trailing /** test but TestApplyGlobsToFilesAndDirs has fewer, different test cases, which don't include trailing /** ? Shouldn't these have the same test cases?

The idea of TestApplyGlobsToFilesAndDirs is to test that files and dirs get removed from the list according to whether they [have descendants that] match globs, not to re-check which glob patterns match which paths. Added a comment about this.

It would be helpful to have a comment explaining that "applyGlobsToFilesAndDirs" filters out files that were found on disk before they are uploaded, whereas "applyGlobsToCollectionFS" removes files from the output collection when it has been pre-populated.

Added.

One of the performance issues we are trying to fix is a situation where the tool has staged 4000 files in 4000 different collections in the working directory. It turns out that even though it is just doing a manifest text copy, loading 4000 collections one by one, at 1 second each, still takes over an hour. This implementation of output_glob would end up doing all that work only to immediately throw it out.

Updated to skip loading collections (and walking subtree mounts) in cases where contents couldn't match globs.

Is there a reason not to use ::json or ::jsonb here?

Mainly just consistency with the other serialized columns.

12430-output-glob @ c1eab853fcda31c546b36d66ca7272ac48ac0756 -- developer-run-tests: #4195

Tom Clegg wrote in #note-39:

Peter Amstutz wrote in #note-38:

The output_glob description is pretty vague. Just as a matter of adequate API documentation, our documentation should probably include a copy of the "Patterns" documentation from doublestar:

https://github.com/bmatcuk/doublestar?tab=readme-ov-file#patterns

Added more detail & examples.

On that note, that section says: A doublestar (**) should appear surrounded by path separators such as /**/. This makes wonder if matching patterns as either files or directories requires ["foo", "foo/*", "foo/**/*"] ? Perhaps a few more test cases would be beneficial?

No, matching either a file or an entire subtree requires just ["foo", "foo/**"].

Added a bunch more test cases, including unit tests that more explicitly check file vs. directory handling.

I see TestApplyGlobsToCollectionFS has a trailing /** test but TestApplyGlobsToFilesAndDirs has fewer, different test cases, which don't include trailing /** ? Shouldn't these have the same test cases?

The idea of TestApplyGlobsToFilesAndDirs is to test that files and dirs get removed from the list according to whether they [have descendants that] match globs, not to re-check which glob patterns match which paths. Added a comment about this.

It would be helpful to have a comment explaining that "applyGlobsToFilesAndDirs" filters out files that were found on disk before they are uploaded, whereas "applyGlobsToCollectionFS" removes files from the output collection when it has been pre-populated.

Added.

One of the performance issues we are trying to fix is a situation where the tool has staged 4000 files in 4000 different collections in the working directory. It turns out that even though it is just doing a manifest text copy, loading 4000 collections one by one, at 1 second each, still takes over an hour. This implementation of output_glob would end up doing all that work only to immediately throw it out.

Updated to skip loading collections (and walking subtree mounts) in cases where contents couldn't match globs.

Is there a reason not to use ::json or ::jsonb here?

Mainly just consistency with the other serialized columns.

12430-output-glob @ c1eab853fcda31c546b36d66ca7272ac48ac0756 -- developer-run-tests: #4195

LGTM. Will probably do some additional integration testing when implementing #9964 but this is ready to merge in order to unblock #9964