Closed wshanks closed 5 months ago
Hmm, I am not sure I would go all the way to block_for_results
with copy()
. I am not sure what the intended uses are for copy()
but I think one is that you want to keep the "input" metadata and run new analysis, so you don't need to wait for the previous analysis? Also, blocking in copy()
would not help with the test failure we had. That issue was that running analysis starts by clearing the results of an experiment data object and it was indeterminate whether that clearing happened before or after the extra add_figures()
was called. I don't understand why a user couldn't run multiple analysis classes on an experiment data and accumulate the results. In the git history, I couldn't find what issue clearing the results was avoiding versus just letting the results get overwritten if they already existed.
That's fair. Specifically for the copy_results=True
case, it calls _wait_for_futures()
instead of block_for_results()
which checks recursively that the futures are all finished, but that doesn't seem to cause any issues. Keeping multiple sets of results does sound potentially useful, so we should discuss changing the behavior for BaseAnalysis.run()
or adding it as an option.
ExperimentData._clear_results()
clearedExperimentData._figures
but not the relatedExperimentData._db_data.figure_names
exposed asExperimentData.figure_names
.BaseAnalysis.run()
callsExperimentData._clear_results()
so when a second analysis run produced a different set of figure names from the first theExperimentData
instance would be left afigure_names
property with entries that did not match the underlying figures data. Here, this issue is addressed by also clearingExperimentData._db_data.figure_names
.This issue was first noticed due to occasional failures of the
test.database_service.test_db_experiment_data.TestDbExperimentData.test_copy_figure_artifacts
test which runs a fake experiment and then adds a figure and artifact to the experiment data. Then the tests copies the experiment data to test the behavior ofcopy()
. Occasionally, the adding of the figure occurs before the background analysis thread runs. In this case, the figure name gets added tofigure_names
before the results get cleared by the analysis callback. Because of the bug described above, the old figure name stays around in the original experiment data object but does not copy to the new object because figure names are copied using the data inExperimentData._figures
and notExperimentData._db_data.figure_names
. Ablock_for_results()
was added to the experiment because in principle thecopy()
could occur before the analysis clears results and then the two data objects would not match, though this case has not been observed (only adding the figure and then clearing it with analysis before copying so the copy is the one missing the figure name has been observed; not the copy having the name and not the original).Addtionally,
BaseAnalysis.run()
handled thefigure_names
option incorrectly when passes a keyword argument. Keyword arguments to passed torun()
cause the analysis class to copy itself and add the options to the copy before running it. However, within therun()
method,self.options.figure_names
was used for assigning figure names rather than referencing the copy of the analysis class, so figure names could not be set usingfigure_names
as a keyword argument. This issues was fixed by replacingself
references with references to the copy.A regression test was added that fails for either of the above issues being present (figure names not matching after re-running analysis; figure names not settable from a
run()
keyword argument).