-
Notifications
You must be signed in to change notification settings - Fork 16
Cannot access result from external python script #47
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
base: master
Are you sure you want to change the base?
Conversation
robertu94
left a comment
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.
See suggested changes.
src/plugins/metrics/composite.cc
Outdated
| struct pressio_options metrics_result; | ||
| for (auto const& plugin : plugins) { | ||
| pressio_options plugin_options = plugin->get_metrics_results({}); | ||
| pressio_options plugin_options = plugin->get_metrics_results(metrics_result); |
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.
This isn't correct, but is probably still a bug. The pressio_metrics_results object passed to get_metrics_results is intended to provide the metrics from the compressor plugin. An example of a compressor plugin that provides this is sz2, which provides metrics on how well the huffman tree and prediction stages worked.
However, here you passed the metrics from other plugins which is incorrect. The right fix here would be to name the argument to this function on L157 and then pass it here.
| int end_decompress_many_impl(compat::span<const pressio_data* const> const& , | ||
| int end_decompress_many_impl(compat::span<const pressio_data* const> const& inputs, | ||
| compat::span<const pressio_data* const> const& outputs, int ) override { | ||
| if(use_many or outputs.size() > 1) { |
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.
don't touch this
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.
inputs for end_decompress_many_impl refers to the compressed data (i.e., a stream of bytes, and is not what you want.
| private: | ||
|
|
||
| std::string qoi = "external:results:data"; | ||
| pressio_metrics child = metrics_plugins().build("noop"); |
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.
While this is correct here (because noop is the default value for meta plugins), you need to configure this with a call to configure qoi:metric to be external in the CLI or user code that you used for testing. I would encourage you to add a test to the test folder that exertises this funtionality, and would also allow me to comment on your testing code.
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.
the way you do this is with -b qoi:metric=external in the command above. This is instead of -m external
My Command and output:
[ziweiq2@ragnaros metrics]$ pressio
-i /home/ziweiq2/LibPressio/dataset/SDRBENCH-EXASKY-NYX-512x512x512/baryon_density.f32
-b compressor=sz3
-o abs=1e-4
-d 512 -d 512 -d 512 -t float
-m external
-o external:command="/home/ziweiq2/halo/run_pressio_pipeline.py --external_mode"
-m qoi
-M all
Registering qoi plugin
composite:compression_rate =
composite:compression_rate_many =
composite:decompression_rate =
composite:decompression_rate_many =
Output of Python Script:
python run_pressio_pipeline.py --external_mode
external:api=json:1
{"external:results:data": 160.72}