New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-28964: add progress reporting #365
Conversation
5d69766
to
b3acd4a
Compare
python/lsst/obs/base/defineVisits.py
Outdated
@@ -325,6 +326,7 @@ def __init__(self, config: Optional[DefineVisitsConfig] = None, *, butler: Butle | |||
super().__init__(config, **kwargs) | |||
self.butler = butler | |||
self.universe = self.butler.registry.dimensions | |||
self.progress = Progress("obs.base.RawIngestTask") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define Visits?
continue | ||
nDatasets = len(datasetsForCalibDate) | ||
self.task.log.info("Ingesting %s %s dataset%s into run %s.", nDatasets, | ||
datasetType.name, "" if nDatasets == 1 else "s", run) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine someone is going to point out that this style of reporting was streamlined by the _log_msg_counter
helper function in ingest.py
. Although I mostly relied on the counter and the "s" being next to each other and not with another string in between. (I use *_log_msg_counter(datasetsForCalibDate)
in the log call and no explicit len()
call here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change it to use that. I admit that left to my own advices I'd be totally content to just write "ingesting %d dataset(s)"
and call it a day.
self.task.registry.registerRun(run) | ||
self.task.butler3.ingest(*datasetsForCalibDate, transfer=self.task.config.transfer, | ||
run=run) | ||
progressBar.update(len(datasetsForCalibDate)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't this length get calculated above?
self._files = [] | ||
self._subdirectories = [] | ||
if log is None: | ||
log = Log.getLogger("obs.base.gen2to3.walker") | ||
self.log = log | ||
self.progress = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this None
and not using the parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub's reaction dropdown list needs 🤦: because I've fixed this at least twice in fixup commits and keep blowing it away as I juggle branches. Glad you caught this one.
55e3f32
to
3d54ef6
Compare
3d54ef6
to
cc77d66
Compare
cc77d66
to
7b2956c
Compare
This doesn't do anything unless the caller uses daf_butler's new
ProgressHandler
interface to install something that can show the progress to the user, but it provides some nice feedback inRawIngestTask
andConvertRepoTask
when that is done (though it's not that useful unless logs are sent to a file instead of the console - nothing gets corrupted, but it's hard to see a progress bar in a sea of INFO logs).