Skip to content
This repository has been archived by the owner on Dec 1, 2021. It is now read-only.

Add device inference test to e2e tests #872

Merged
merged 30 commits into from
Mar 31, 2020
Merged

Add device inference test to e2e tests #872

merged 30 commits into from
Mar 31, 2020

Conversation

hadusam
Copy link
Contributor

@hadusam hadusam commented Feb 20, 2020 •
edited
Loading

What this patch does to fix the issue.

A part of #854
Added test on ubuntu(x86), ubuntu(x86_avx), raspbery-pi(aarch64), de10-nano(arm)
I also tried to add de10-nano(fpga), but the inference was hanged up. I think it is caused by our test configs so I decided to fix and add test for inference on fpga after this PR.

Link to any relevant issues or pull requests.

After #870 and #871

@hadusam hadusam self-assigned this Feb 20, 2020
@blueoil-butler blueoil-butler bot added the CI: auto-run Run CI automatically label Feb 20, 2020
@hadusam hadusam force-pushed the add_test_for_device_inference branch 2 times, most recently from 40354f4 to e8f2179 Compare February 21, 2020 08:20
@hadusam hadusam force-pushed the add_test_for_device_inference branch from a21b695 to 3969633 Compare March 26, 2020 04:36
@hadusam hadusam added the CI: test-blueoil Run blueoil test once label Mar 26, 2020
@blueoil-butler blueoil-butler bot removed the CI: test-blueoil Run blueoil test once label Mar 26, 2020
@hadusam hadusam marked this pull request as ready for review March 26, 2020 12:35
@bo-code-review-bot
Copy link

This PR needs Approvals as follows.

  • Ownership Approval for / from iizukak, tkng, ruimashita
  • Readability Approval for Python from tkng, tsawada

Please choose reviewers and requet reviews!

Click to see how to approve each reviews

You can approve this PR by triggered comments as follows.

  • Approve all reviews requested to you (readability and ownership) and LGTM review
    Approval, LGTM

  • Approve all ownership reviews
    Ownership Approval or OA

  • Approve all readability reviews
    Readability Approval or RA

  • Approve specified review targets

    • Example of Ownership Reviewer of /: Ownership Approval for / or OA for /
    • Example of Readability Reviewer of Python: Readability Approval for Python or RA for Python

  • Approve LGTM review
    LGTM

See all trigger comments

Please replace [Target] to review target

  • Ownership Approval
    • Ownership Approval for [Target]
    • OA for [Target]
    • Ownership Approval
    • OA
    • Approval
  • Readability Approval
    • Readability Approval for [Target]
    • RA for [Target]
    • [Target] Readability Approval
    • [Target] RA
    • Readability Approval
    • RA
    • Approval
  • LGTM
    • LGTM
    • lgtm

@hadusam hadusam requested review from iizukak and tfujiwar March 26, 2020 12:35
@hadusam hadusam removed request for iizukak and tfujiwar March 27, 2020 02:05
@hadusam hadusam changed the title Add device inference test to e2e tests [WIP] Add device inference test to e2e tests Mar 27, 2020
tfujiwar
tfujiwar approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@tfujiwar tfujiwar left a comment

Choose a reason for hiding this comment

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

Readability Approval (reverse-shadowing)

@hadusam hadusam force-pushed the add_test_for_device_inference branch from 7e6447a to bcaddf3 Compare March 27, 2020 02:26
@hadusam hadusam force-pushed the add_test_for_device_inference branch from bcaddf3 to 152d411 Compare March 27, 2020 08:00
@hadusam hadusam changed the title [WIP] Add device inference test to e2e tests Add device inference test to e2e tests Mar 27, 2020
@hadusam hadusam force-pushed the add_test_for_device_inference branch from 1cc3237 to a7d8632 Compare March 27, 2020 15:08
@hadusam
Copy link
Contributor Author

hadusam commented Mar 27, 2020

@tfujiwar
I changed my mind to use python for testing instead of shell.
Please review this again! Thanks!

@hadusam hadusam requested a review from tfujiwar March 27, 2020 15:10
@hadusam hadusam added the CI: test-dlk Run dlk test once label Mar 27, 2020
@blueoil-butler blueoil-butler bot removed the CI: test-dlk Run dlk test once label Mar 27, 2020
tfujiwar
tfujiwar reviewed Mar 30, 2020
View reviewed changes
Copy link
Contributor

@tfujiwar tfujiwar 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 contribution!
I left some comments.

tests/device_tests/test_device_e2e.py Outdated
if output_dir:
output_dir = output_dir[0]
else:
raise Exception("No such directory")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please think about more specific exceptions. In this case, FileNotFoundError would be appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tfujiwar
Thanks!
I tried to use it at first but there is not FileNotFoundError of python2. So I use a custom exception. But it is better to use IOError for python2, so I changed it with if statement.
Please check it again!

tests/device_tests/test_device_e2e.py Outdated
sys.path.append(python_path)
from run import run_prediction
run_prediction(image, model, config)
assert os.path.exists(os.path.join('output', "output.json"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use self.assertTrue in unittest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

tests/device_tests/test_device_e2e.py Outdated
return [[case, self._get_param(case, input_path, lib_name)] for case in os.listdir(input_path)]

def _run(self, python_path, image, model, config):
sys.path.append(python_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need to initialize sys.path for every test case to avoid the conflict of the module name run among all test cases.

class DeviceE2eTest(unittest.TestCase):
    sys_path_default = copy.deepcopy(sys.path)

    def _run():
        sys.path = copy.deepcopy(sys_path_default) + [python_path]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tfujiwar
Nice review! Thanks a lot! It would cause a bug, I didn' t noticed.
I fixed it 👍

iizukak
iizukak approved these changes Mar 31, 2020
View reviewed changes
Copy link
Member

@iizukak iizukak left a comment

Choose a reason for hiding this comment

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

OA

@iizukak
Copy link
Member

iizukak commented Mar 31, 2020

/ready

@bo-mergebot
Copy link
Contributor

bo-mergebot bot commented Mar 31, 2020

⏳Merge job is queued...

@bo-mergebot
Copy link
Contributor

bo-mergebot bot commented Mar 31, 2020

😥This PR is not approved yet.

@hadusam hadusam requested a review from tfujiwar March 31, 2020 02:07
tfujiwar
tfujiwar approved these changes Mar 31, 2020
View reviewed changes
Copy link
Contributor

@tfujiwar tfujiwar left a comment

Choose a reason for hiding this comment

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

Readability Approval
Thank you!!

@hadusam
Copy link
Contributor Author

hadusam commented Mar 31, 2020

/ready

@bo-mergebot
Copy link
Contributor

bo-mergebot bot commented Mar 31, 2020

⏳Merge job is queued...

@bo-mergebot bo-mergebot bot merged commit 883204d into blue-oil:master Mar 31, 2020
oatawa1 pushed a commit to oatawa1/blueoil that referenced this pull request Mar 31, 2020
@hadusam @oatawa1
Add device inference test to e2e tests (blue-oil#872)
dcf40a1
oatawa1 pushed a commit to oatawa1/blueoil that referenced this pull request Mar 31, 2020
@hadusam @oatawa1
Add device inference test to e2e tests (blue-oil#872)
e21d0ae
@hadusam hadusam deleted the add_test_for_device_inference branch April 1, 2020 02:45
@hadusam hadusam mentioned this pull request Apr 1, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iizukak iizukak iizukak approved these changes

@tfujiwar tfujiwar tfujiwar approved these changes

Assignees

@hadusam hadusam

Labels
CI: auto-run Run CI automatically
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hadusam @iizukak @tfujiwar