DM-15465: Clean up utils usage with a view to supporting test running in pull requests #323
Conversation
timj
force-pushed
the
tickets/DM-15465
branch
4 times, most recently
from
2b0f9f0
to
81eb9d9
Compare
|
This now seems to be passing so that pull requests will run the tests and build the documentation. It takes 8minutes to run and is currently not required for merging. Two minutes of that is building sphgeom so maybe we can upload a wheel as a "GitHub release" and pull that to save some time. I wonder if we have to reconsider the pull_request and push builds. Are they the same once a pull request has been created? |
There was a problem hiding this comment.
Only big picture comment is that with all of the conditionals in ButlerURI I wonder if it'd work better as an ABC with separate implementations for remote URIs, POSIX paths, and now pkg_resources. Or maybe a concrete class that delegated to an ABC or something. Probably out of scope for this ticket, but something to think about.
|
Regarding ButlerURI returning subclasses for different URI schemes, that does seem worth thinking about. It would also allows the addition of more concrete methods such as "does this item exist" and "download this resource". |
| - name: Run tests | ||
| run: pytest -r a -v -n 3 | ||
|
|
||
| - name: Install documenteer |
There was a problem hiding this comment.
Note for the future: Should package the install and build as an action that others can reuse.
| - name: Install sphgeom dependencies | ||
| run: pip install -r https://raw.githubusercontent.com/lsst/sphgeom/master/requirements.txt | ||
|
|
||
| - name: Install dependencies |
There was a problem hiding this comment.
Note for the future: these four steps (deps, xdist, install ., pytest) could be packaged as an action for others to reuse.
There was a problem hiding this comment.
An action can import another action?
There was a problem hiding this comment.
A workflow (which this is) uses actions. I'm saying to package up the run scripts as a reusable action.
| @@ -140,6 +148,66 @@ def extractFile(self, filename): | |||
| return yaml.load(response["Body"], Loader) | |||
|
|
|||
|
|
|||
| @dataclass | |||
| class Resource: | |||
There was a problem hiding this comment.
Is there a way to unify filesystem paths or even URIs with these Resource paths so that they appear to the rest of the code as a uniform class instead of having to type-case everywhere?
There was a problem hiding this comment.
Adding pkg_resources://lsst.daf.butler/configs/datastore.yaml URI support to ButlerURI might work but really needs ButlerURI to be updated to have concrete methods for checking existence and even opening a stream. I think some of this ties in with @TallJimbo wondering if ButlerURI could be changed so that the constructor returns specialist S3ButlerURI, PosixFileURI and LocalFileURI implementations.
Make use of new APIs in ButlerURI
This required that the configs move inside the python package. It also needed some cleanup in ButlerURI versus bare paths and the addition of support to instantiate a Config from a new Resource class.
No other daf_butler tests include the memory tester so the usage here was anomalous.
Python really really wants it there and setuptools won't work without it. It already caused problems with mypy so put it back.
If lsst.utils is not available locally the files we need are downloaded.
Not sure why the next line doesn't do this
click 7.0 uses double quotes but 7.1 uses a single quote. For now we support both so switch the code to a regex rather than a substring match.
Co-authored-by: Kian-Tat Lim <ktl@slac.stanford.edu>
Co-authored-by: Kian-Tat Lim <ktl@slac.stanford.edu>
We were actually losing our override and getting a default sqlite registry file.
This saves a special "str" logic path.
Disable .ttf files opened by matplotlib.
Still have many doImport usages and one test uses a getTempFilePath call.