search

plaidml / openvino

OpenVINO™ Toolkit - Deep Learning Deployment Toolkit repository
https://docs.openvinotoolkit.org/latest/index.html
Apache License 2.0
0 stars 1 forks source link

Update OV #155

Closed YangleiZouIntel closed 3 years ago

YangleiZouIntel commented 3 years ago

Update OV to the last commit on May 30, 2021

tzerrell commented 3 years ago

I think this PR would not be well-served by squashing; I think we want to retain the separate upstream commits unsquashed. To that end, let's not land this directly. I plan to update the plaidml branch to the HEAD of this branch and then push to this repo to get this update landed (probably tomorrow).

YangleiZouIntel commented 3 years ago

I think this PR would not be well-served by squashing; I think we want to retain the separate upstream commits unsquashed. To that end, let's not land this directly. I plan to update the plaidml branch to the HEAD of this branch and then push to this repo to get this update landed (probably tomorrow).

Thanks. Yes, by squash and merge, all the commits merged into one. I merged upstream to this branch so it can be merged to plaidml-v1 without conflicts ( However, as you said the commit history is not well retained). Updating the plaidml branch to the HEAD of this branch is much better.

If you prefer to rebase this branch onto latest OV master (instead of merge), I can rebase to the same upstream commit. Please let me know.

Separately, @XinWangIntel 's PR https://github.com/openvinotoolkit/openvino/pull/5714 which fixed ActivationParamLayerTest has merged to upstream yesterday (Not included in this PR). There is a complete fix in https://github.com/openvinotoolkit/openvino/pull/5915 from an OV developer which was approved but not yet merged, If you prefer to included it in this update, please also let us know. I guess we will need to wait 1 or 2 days for it to be merged before we can include it in this update.

tzerrell commented 3 years ago

Ok, thanks for the update. Let's get at least one of those fixes into this PR before merging. If I understand correctly, the more complete fix doesn't particularly affect us? If that's correct there's no need to wait for it IMO.

Also, no need to rebase, the current commit structure you have seems reasonable to me.

YangleiZouIntel commented 3 years ago

Ok, thanks for the update. Let's get at least one of those fixes into this PR before merging. If I understand correctly, the more complete fix doesn't particularly affect us? If that's correct there's no need to wait for it IMO.

Also, no need to rebase, the current commit structure you have seems reasonable to me.

I have updated this branch to https://github.com/openvinotoolkit/openvino/commit/5c716d2afc411c69e8bca9ec82a1290aec245c25, which includes Xin's fix.

For your question, Xin's fix was based on the code before update. In order to pass this activation tests after updated OV, probably we will need to include the complete fix . Since that test is commented out and that complete fix seems to still be under review status, I think we do not need to wait for it.

tzerrell commented 3 years ago

Merged as discussed above, thanks!