DM-10779: Implement running time metric(s) #3

kfindeisen · 2017-08-10T23:51:15Z

This PR adds a function for extracting running time from Task metadata, and adds some infrastructure to ap_verify to support it and future features.

ebellm

Just three high-level comments:

I don't want us wrapping ap_pipe libraries in a special ApPipe object. I'd rather we call the required library functions directly from run_ap_verify().

I think slightly more descriptive names for api.py and performance.py would be useful.

ebellm · 2017-08-18T05:22:02Z

python/lsst/ap/verify/measurements/api.py

+# see <http://www.lsstcorp.org/LegalNotices/>.
+#
+
+"""Code for measuring software performance metrics.


Could this be called something more descriptive than "api.py?"

I couldn't think of one, and still can't. This file's purpose is to provide a single point of entry to the measurements sub-package, so that ap_verify.py doesn't need to know about implementation details like "which measurements have actually been implemented"?

Is there a name that communicates that to you?

What about compute_metrics? "API" suggests to me a library with multiple methods, not an abstraction layer.

A top-level docstring which better indicated the purpose of the package would help here--the code for measuring metrics is not actually in this file.

ebellm · 2017-08-18T05:22:34Z

python/lsst/ap/verify/measurements/performance.py

+
+import lsst.verify
+
+


similarly, could this be called something more descriptive than "performance.py?"

Such as? measurements/performance.py seems to quickly summarize what the code is for.

It depends where you plan to put other metrics code--it's all "performance" at some level. If you're thinking of small packages, runtime_metrics.py would be more clear.

"runtime" to me sounds like "not compile-time", but I see your point. How about profiling.py?

(I'd prefer to avoid 'metrics' or 'measurement' in the name to avoid Department of Redundancy Department).

okay, that works for me.

kfindeisen · 2017-08-18T15:35:58Z

I am willing to replace the ApPipe class with the function-based module that came before it -- the former was a request by @djreiss in the previous review that, as Lupton warned us to watch out for yesterday, constitutes scope creep. Note that the ap_pipe wrapper is outside the scope of this ticket, so it may be easier (particularly for @mrawls) if we create a new ticket to make that change.

However, I must strongly object to dumping the ApPipe code into run_ap_verify. This would not only bloat that function well beyond its already long length and make the code more complicated, but add extra, implementation-specific dependencies between the main ap_verify module and ap_pipe. If we want any hope of having ap_verify be easy to maintain and improve in the future, everything that depends on ap_pipe needs to be in its own module, safely compartmentalized from the rest of the system rather than mixed in with code related to argument and metrics management

ebellm · 2017-08-18T16:45:29Z

Okay, I'll let you refactor the code to be more functional, but I don't agree that ap_verify needs to be strictly compartmentalized from the details of ap_pipe--it should just be a small wrapper to add the metrics and verification support. I'd prefer tighter coupling to keep the package lightweight.

Metadata will be used to recover Measurements from individual pipeline Tasks, and to compute "afterburner" metrics. The Pipeline API has been changed to support metadata, and a method has been added to Pipeline to support current plans for Measurement handling.

Basic parsing is delegated to daf.persistence.Policy. The config loading has been factored into a singleton class to ensure options are only loaded once while decoupling it from other ap_verify code.

The measurement code has been put in a subpackage, measurements, which is expected to have many other measurements added to it in the future.

Package was not ready to add before.

This change reduces the conceptual complexity of ap_verify, and makes the data flow more obvious. Dependencies on the ap_pipe API are still contained in a separate module, where they can't clutter up the top-level logic.

kfindeisen requested a review from ebellm August 10, 2017 23:51

kfindeisen force-pushed the tickets/DM-10779 branch 3 times, most recently from 1d37418 to 3e7cca1 Compare August 14, 2017 16:09

ebellm requested changes Aug 18, 2017

View reviewed changes

kfindeisen force-pushed the tickets/DM-10779 branch 2 times, most recently from a9e032f to a4f0c47 Compare August 24, 2017 23:48

kfindeisen and others added 7 commits August 31, 2017 13:51

Modernize config handling.

182f326

Basic parsing is delegated to daf.persistence.Policy. The config loading has been factored into a singleton class to ensure options are only loaded once while decoupling it from other ap_verify code.

Implement runtime measurements.

feddc8b

The measurement code has been put in a subpackage, measurements, which is expected to have many other measurements added to it in the future.

Cleanup package boilerplate.

8a3f91f

Move main ap_verify executable to bin.src

59bc252

Add ap_pipe as a dependency.

cfafd0a

Package was not ready to add before.

Revert making pipeline driver a class.

25eb227

This change reduces the conceptual complexity of ap_verify, and makes the data flow more obvious. Dependencies on the ap_pipe API are still contained in a separate module, where they can't clutter up the top-level logic.

kfindeisen force-pushed the tickets/DM-10779 branch from a4f0c47 to 25eb227 Compare August 31, 2017 21:04

kfindeisen merged commit 25eb227 into master Aug 31, 2017

kfindeisen deleted the tickets/DM-10779 branch November 30, 2018 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10779: Implement running time metric(s) #3

DM-10779: Implement running time metric(s) #3

kfindeisen commented Aug 10, 2017

ebellm left a comment

ebellm Aug 18, 2017

kfindeisen Aug 18, 2017

ebellm Aug 18, 2017

ebellm Aug 18, 2017

kfindeisen Aug 18, 2017

ebellm Aug 18, 2017

kfindeisen Aug 18, 2017 •

edited

ebellm Aug 18, 2017

kfindeisen commented Aug 18, 2017 •

edited

ebellm commented Aug 18, 2017


		import lsst.verify

DM-10779: Implement running time metric(s) #3

DM-10779: Implement running time metric(s) #3

Conversation

kfindeisen commented Aug 10, 2017

ebellm left a comment

Choose a reason for hiding this comment

ebellm Aug 18, 2017

Choose a reason for hiding this comment

kfindeisen Aug 18, 2017

Choose a reason for hiding this comment

ebellm Aug 18, 2017

Choose a reason for hiding this comment

ebellm Aug 18, 2017

Choose a reason for hiding this comment

kfindeisen Aug 18, 2017

Choose a reason for hiding this comment

ebellm Aug 18, 2017

Choose a reason for hiding this comment

kfindeisen Aug 18, 2017 • edited

Choose a reason for hiding this comment

ebellm Aug 18, 2017

Choose a reason for hiding this comment

kfindeisen commented Aug 18, 2017 • edited

ebellm commented Aug 18, 2017

kfindeisen Aug 18, 2017 •

edited

kfindeisen commented Aug 18, 2017 •

edited