tickets/PREOPS-4685: use lsst.resources for loading data in the prenight briefing #59
Conversation
|
This pull request only covers the actual loading of data. At present, the dashboard just presents a dropdown of all matching files in the resource, an interface which is clearly not scalable to the numbers of simulations in the data store we will get. Improving the interface to scale to large numbers of simulations will be covered in a different jira issue (PREOPS-4697) and corresponding pull request. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #59 +/- ##
==========================================
+ Coverage 12.70% 13.08% +0.38%
==========================================
Files 40 42 +2
Lines 3219 3263 +44
Branches 479 488 +9
==========================================
+ Hits 409 427 +18
- Misses 2801 2827 +26
Partials 9 9 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
So .. I didn't actually try it (I'm going to assume the functionality is something you've tested and that it works). Seems ok to me otherwise. I don't have an S3 bucket to attempt to retrieve from, nor things on disk (or rather, I do have stuff on disk, but they're all in the expected location, so it's harder to know if the "test" succeeded or if it just defaulted to something that worked).
| @@ -34,6 +34,7 @@ dependencies = [ | |||
| "param", | |||
| "pytz", | |||
| "rubin-scheduler", | |||
| "lsst.resources", | |||
There was a problem hiding this comment.
I think you're right that lsst.resources will install from pypi, but I'm guessing that for conda-forge it will have to be lsst-resources (and I'd like to ask for a conda-forge package).
And also, lsst-resources does work for pypi as well, so maybe it'd be good to just simply use "lsst-resources" instead of lsst.resources in one place and lsst-resources in another?
(and I also agree that for pyproject.toml it makes no difference, given how pip checks .. it's entirely for looking 'the same').
| default=default_data_dir, | ||
| help="The base directory for data files.", | ||
| default=DEFAULT_RESOURCE_URI, | ||
| help="The base URI for data files.", |
There was a problem hiding this comment.
Can this still be just a directory on disk? (if so, might be helpful to say something like that .. "The base URI for data file. This could be the S3 bucket or a directory on disk." ??
There was a problem hiding this comment.
It will definitely take a string like file://home/someuser/data/foo. That's one of the ways I've been testing it. I think it may also take /home/someuser/data/foo, but I'm not sure.
Right now, it's only been tested on files in the local filesystem and package resources, but hopefully it will work with s3 buckets as well, when there is one with opsim data.