refactoring load() to allow caller to override file-reading logic

[mlin]

as planned in #177 discussion

[mlin]

@dinvlad WDYT of this?

I hope making the read_source callback an async one will not be too inconvenient; this maximizes the flexibility since it'll often be used to read from network URIs.

[coveralls]

Pull Request Test Coverage Report for Build 910

• 85 of 90 (94.44%) changed or added relevant lines in 4 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.01%) to 96.873%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
WDL/init.py	6	7	85.71%
WDL/Tree.py	27	29	93.1%
WDL/_parser.py	51	53	96.23%

Totals
Change from base Build 901:	-0.01%
Covered Lines:	4058
Relevant Lines:	4189

---

💛 - Coveralls

[mlin]

@dinvlad I want to highlight the above logic related to handling nested relative imports, which might become tricky for the LSP get_document() use case. For example suppose we have a directory tree,

A
├── a.wdl
└── B
    ├── b.wdl
    └── C
        └── c.wdl

and A/a.wdl does import B/b.wdl, b.wdl does import C/c.wdl. When asked to read_source("C/C.wdl") one has to figure out that's relative to B/ not A/.

[dinvlad]

Hmm, so the uri parameter to read_source is verbatim from import, not a fully resolved absolute path? I thought we could just the uri to LSP if it was a full file://A/B/C/c.wdl.

But if it's relative, we'll have to essentially use everything from read_source_default() until with open(fn, "r") as infile:, to fully resolve the uri but not to read it (as reading will be done from memory in LSP, or from file system if the uri is not in memory).

So given that we have to essentially reuse all but the last couple lines in read_source_default(), would it perhaps make sense to move the "full URI resolution" functionality into a separate function, and then to provide a read_source_uri(uri) method to actually implement loading from a fully-resolved URI?

[dinvlad]

And for a network URI, we don't have to resolve it so it will be just short-cut by read_source_default(). So making the code in read_source_default() run always first, before calling read_source_uri(), could be a general solution :)

[dinvlad]

I hope making the read_source callback an async one will not be too inconvenient; this maximizes the flexibility since it'll often be used to read from network URIs.

Agreed on this point!
Please see above for the uri stuff.

[mlin]

@dinvlad thanks for the feedback! I did more work on it to additionally store the absolute path after resolution of relative loads/imports. The diff is getting pretty unwieldy now but the key docstring:

https://github.com/chanzuckerberg/miniwdl/pull/205/files#diff-c7ebedebf9b171179223458e04e073dcR91

and upshot:

1. read_source is now responsible for returning the absolute filename/URI of the document read, in addition to its contents.
2. the pos: SourcePosition attribute on all AST nodes including Document now has separate uri and abspath attributes, the first being whatever was passed initially and the second being the resolved absolute path.
3. read_source now gets importer: Document instead of importer_uri: str, so in particular it has importer.pos.{uri,abspath}. This permitted simplifying the logic for handing relative paths in read_source_default. (Will it still help further to break this out into something you can call on its own?)

[dinvlad]

Hi @mlin, I'm not sure if this addresses our use case directly, if I understood the signature of read_source correctly. Could you clarify, does MiniWDL invoke read_source for each file that it wants to get the source text for? In that case, we have to make it resolve text corresponding to the fully qualified uri, not importer.pos.uri, because we aren't asking to resolve text for the importer uri, only for the imported uri. Does that make sense? Thanks

[dinvlad]

In addition, which function can we call to fully resolve the URI first (that is, to make it absolute file:// or keep it as http(s):// URI)?

To re-iterate, what I was asking for is:

1. Provide a general resolve_uri function that resolves relative or HTTP URI into absolute or HTTP URI, where in case of HTTP URI it just returns the URI as is. This function should NOT attempt to also resolve the text.

   In the case of LSP, we don't want to re-implement this function, and would like to just re-use its default implementation.

2. Add a read_source function (as an option to load or load_async) that takes in an absolute URI (file:// or http(s)://) and resolves it into text.

   This function we will override with our implementation for LSP, which can be as simple as

   return ls.workspace.get_document(absolute_uri).source

Would it make sense to split functionality in this way, or did I misunderstand the intent of the current read_source?

[mlin]

@dinvlad ok, sold :) I made read_source_default call this, rather than taking its result as an argument, in order to avoid having to make them both overridable load() arguments

[dinvlad]

Perfect, thanks!!

[dinvlad]

Hi @mlin,

I just realized this is still not working as expected. E.g. when it parses Optimus.wdl for import "FastqToUBam.wdl" as FastqToUBam, read_source() receives FastqToUBam.wdl as the uri, instead of the fully-resolved URI as we discussed. Is it possible to add functionality such that all relative imports would be fully-resolved first, before passing them to read_source()?

Thanks

[dinvlad]

Ok, I was able to achieve this functionality using

uri = await WDL.Tree.resolve_file_import(uri, path, importer)

I forgot that this is how we decided to do it :-) I guess that'd still be nice to do automatically instead, but not critical.

[dinvlad]

Btw, I also noticed that WDL.resolve_file_import() is not currently implemented yet. I had to use WDL.Tree.resolve_file_import() instead.