Skip to content
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

Add --output parameter to demo to save results #2005

Merged
merged 11 commits into from
Apr 14, 2023

Conversation

GalyaZalesskaya
Copy link
Contributor

@GalyaZalesskaya GalyaZalesskaya commented Apr 12, 2023

Summary

  • Added the ability to save inference images in OTX CLI demo, exportable demo with --save-results-to parameter.
    It helps to keep and check the inference results after running a demo, using the command line on a remote station, without the necessity to use X server.
  • Checked for images, videos, image folders for Classification task

Jira ticket: https://jira.devtools.intel.com/browse/CVS-107856

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have added e2e tests for validation.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@GalyaZalesskaya GalyaZalesskaya requested a review from a team as a code owner April 12, 2023 19:15
@github-actions github-actions bot added API Any changes in OTX API CLI Any changes in OTE CLI DOC Improvements or additions to documentation labels Apr 12, 2023
@GalyaZalesskaya GalyaZalesskaya added this to the 1.2.0 milestone Apr 12, 2023
@GalyaZalesskaya GalyaZalesskaya added the FEATURE New feature & functionality label Apr 12, 2023
@GalyaZalesskaya
Copy link
Contributor Author

Actually, do I understand correctly, that this parameter doesn't require adding extra tests?

Copy link
Contributor

@eunwoosh eunwoosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work! I left some comment. please take a look

otx/api/usecases/exportable_code/visualizers/visualizer.py Outdated Show resolved Hide resolved
otx/api/usecases/exportable_code/visualizers/visualizer.py Outdated Show resolved Hide resolved
otx/api/usecases/exportable_code/visualizers/visualizer.py Outdated Show resolved Hide resolved
otx/api/usecases/exportable_code/visualizers/visualizer.py Outdated Show resolved Hide resolved
otx/api/usecases/exportable_code/visualizers/visualizer.py Outdated Show resolved Hide resolved
otx/api/usecases/exportable_code/visualizers/visualizer.py Outdated Show resolved Hide resolved
otx/api/usecases/exportable_code/visualizers/visualizer.py Outdated Show resolved Hide resolved
@@ -39,12 +39,13 @@ def run(self, input_stream: Union[int, str], loop: bool = False) -> None:
next_frame_id_to_show = 0
stop_visualization = False

for frame in streamer:
for (frame, input_path) in streamer:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can raise an error if streamer is ThreadStreamer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can stick to a pre-defined output video/images names to avoid introducing the additional output from streamer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it will be much easier. But it would cause a problem, that input and output names aren't consistent and it's hard to identify, which image was the source, based on naming

But if additional output in streamer is highly unwelcome, I can write a standalone function to get input names from args.input, without changing the streamer code.

otx/cli/tools/demo.py Outdated Show resolved Hide resolved
@@ -39,12 +39,13 @@ def run(self, input_stream: Union[int, str], loop: bool = False) -> None:
next_frame_id_to_show = 0
stop_visualization = False

for frame in streamer:
for (frame, input_path) in streamer:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can stick to a pre-defined output video/images names to avoid introducing the additional output from streamer.

Copy link
Contributor

@sovrasov sovrasov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update README.md in the exportable code package according to comments from customer and the latest changes in CLI?

@GalyaZalesskaya GalyaZalesskaya changed the title Add save-results-to parameter to demo Add --output parameter to demo to save results Apr 13, 2023
@GalyaZalesskaya
Copy link
Contributor Author

@eunwoosh @harimkang @sovrasov Thank you for your very useful comments. I removed extra output for the streamer and moved logic for saving images to one function in utils that CLI demo and exportable code demo share between each other.

@sovrasov I updated the exportable code README according to the latest CLI changes and customer feedback. The rest of the changes with exportable code from the feedback will be added in a separate PR to speed up the review of this one.

harimkang
harimkang previously approved these changes Apr 13, 2023
Copy link
Contributor

@harimkang harimkang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
BTW, Do you have any plan to add some test cases for new functions in demo/?

if isinstance(input_path, int):
return []
if "DIR" in capture.get_type():
return [f.name for f in Path(input_path).iterdir() if f.is_file()]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's for DirStreamer right? Then, I think this and that in DirStreamer should have same order.
I think one way to ensure that is a making a function to make a list of files in directory and make both of them use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I didn't quite understand you. Can you please elaborate?

Copy link
Contributor

@eunwoosh eunwoosh Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, function get_input_names_list is in charge of providing list of names to save the frames.
And if "DIR" in capture.get_type(): is for the case where DirStreamer is used. The problem is that each frame doesn't have information about which file is used for current frame in that case. So, when saving the frame, get filenames by [f.name for f in Path(input_path).iterdir() if f.is_file()] and then using each file name iterating zip(filenames, saved_frames).
What I think as problem is an order of [f.name for f in Path(input_path).iterdir() if f.is_file()] in the function and order of sorted(os.listdir(self.dir)) in DirStreamer can be different. So, I think both order of list should be same.
Of course, I may be misunderstanding. then I really appreciate if you let me know :)

@GalyaZalesskaya
Copy link
Contributor Author

LGTM, BTW, Do you have any plan to add some test cases for new functions in demo/?

Sure, I'll add tests on Monday in this PR or in the separate PR (if we'll be able to merge it today)

Copy link
Contributor

@goodsong81 goodsong81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the update. Could you please submit a PR with some tests?

@goodsong81 goodsong81 merged commit 81ba925 into openvinotoolkit:develop Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Any changes in OTX API CLI Any changes in OTE CLI DOC Improvements or additions to documentation FEATURE New feature & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants