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

[Good First Issue]: Support aten::expm1 for pytorch models #20578

Closed
mvafin opened this issue Oct 18, 2023 · 30 comments · Fixed by #22642
Closed

[Good First Issue]: Support aten::expm1 for pytorch models #20578

mvafin opened this issue Oct 18, 2023 · 30 comments · Fixed by #22642
Assignees
Labels
category: PyTorch FE OpenVINO PyTorch Frontend good first issue Good for newcomers no_stale Do not mark as stale
Milestone

Comments

@mvafin
Copy link
Contributor

mvafin commented Oct 18, 2023

Context

OpenVINO component responsible for support of PyTorch models is called as PyTorch Frontend (PT FE). PT FE converts a model represented as TorchScript model to a model in OpenVINO opset.

What needs to be done?

  • Implement conversion rule and/or transformation to support the new operation.
  • Implement operation tests in tests/layer_tests/pytorch_tests. Please consider different data types, but keep reasonable number of test cases

Example Pull Requests

#18998

Resources

Contact points

@openvinotoolkit/openvino-pytorch-frontend-maintainers

Ticket

TBD

@mvafin mvafin added good first issue Good for newcomers no_stale Do not mark as stale labels Oct 18, 2023
@github-project-automation github-project-automation bot moved this to Contrubutors needed in Good first issues Oct 18, 2023
@ilya-lavrenov ilya-lavrenov added the category: PyTorch FE OpenVINO PyTorch Frontend label Oct 18, 2023
@Mac16661
Copy link

@ilya-lavrenov and @mvafin I would like to work on it.

@ilya-lavrenov ilya-lavrenov moved this from Contributors Needed to Assigned in Good first issues Oct 29, 2023
@Mac16661
Copy link

Mac16661 commented Nov 4, 2023

Hi, do u guys have any official discord channel or something ?

@mlukasze
Copy link
Contributor

mlukasze commented Nov 6, 2023

Hi, do u guys have any official discord channel or something ?

not yet, sorry

@p-wysocki
Copy link
Contributor

Hi @Mac16661, do you need any help?

@Mac16661
Copy link

Actuall yes, Im having some trouble setting up this project in my PC

@p-wysocki
Copy link
Contributor

Could you please share what the issues are?

@Mac16661
Copy link

Mac16661 commented Nov 16, 2023

@p-wysocki I fixed that issue thanks to stack overflow. Just tell me one thing do I also need to write Unit test for this function?

@p-wysocki
Copy link
Contributor

That's great! Yes, tests are required - you can see how they can be added in the example PR: https://github.com/openvinotoolkit/openvino/pull/18998/files#diff-95d21d5f44f53bb598a50b564360938a47c9495c4ba2cccd65533f1d33b9086e

@Mac16661
Copy link

Mac16661 commented Nov 18, 2023

Could you provide the ID of any of your social media accounts if you don't mind, so I can clear my doubts? I have a lot of small ones. Like discord or something .

@Mac16661
Copy link

@p-wysocki do u have any command for running these testcases ? Like Im trying to run \openVINO\openvino\tests\layer_tests\pytorch_tests\test_pow.py

@p-wysocki
Copy link
Contributor

Hello @Mac16661!

Could you provide the ID of any of your social media accounts if you don't mind, so I can clear my doubts? I have a lot of small ones. Like discord or something .

We currently do not have any social media for contributors, but that may (I hope) change soon. In the meantime, please ask your questions here, no matter how small they are. :)

@p-wysocki do u have any command for running these testcases ? Like Im trying to run \openVINO\openvino\tests\layer_tests\pytorch_tests\test_pow.py

You can start with the PyTorch Layer Test guide - let us know if you have questions which are not covered by it!

@Mac16661
Copy link

Mac16661 commented Dec 2, 2023

@p-wysocki I have some like, I am implementing expm1 method. So I need to get an input vector and then find it's exponential by calling Exp () and then create an vector of same size as input vector contains 1 and then call Subtract ()
passing input vector and new vector filled with 1 as params.

OutputVector translate_expm1(const NodeContext& context, bool out=false) {
    //Getting input vector
    auto vec = context.get_input(0);

    auto exp = context.mark_node(std::make_shared<ov::op::v0::Exp>(vec)); //calculating exp

    return {context.mark_node(std::make_shared<ov::op::v0::Exp>(vec))}; 
     
}

This is how I am trying to implement this function but I don't know how to create a vector? or what this context.mark_node doing?

So do u have any docs which can help me ?

@mlukasze
Copy link
Contributor

mlukasze commented Dec 2, 2023

@mmikolajcz could you please take a look?

@mvafin
Copy link
Contributor Author

mvafin commented Dec 7, 2023

@Mac16661 You can see on this line how subtract 1 is done in reference PR.
You need to create a Constant with value 1 and call Subtract operation.
More documentation about frontend is available here.
context.mark_node is internal operation that you need to call for each nodes created inside conversion rule, currently it is responsible for assigning names for those nodes, it may be doing more in future.

@p-wysocki
Copy link
Contributor

Hello @Mac16661, do you need any help? Does the provided solution work for you?

Just yesterday our CONTRIBUTING.md has been updated with a technical guide - I highly recommend checking it out. :)

@Mac16661
Copy link

I've implemented expm1 method and unit test is also read, but I'm facing some issue in setting up openvino locally. I just need bit more time

@p-wysocki
Copy link
Contributor

Hello @Mac16661, please let us know if there's anything we can help you with. Are you still working on that issue?

I am happy to announce that we have created a channel dedicated to Good First Issues support on our Intel DevHub Discord server! Join it to receive support, engage in discussions, ask questions and talk to OpenVINO developers.

@p-wysocki p-wysocki moved this from Assigned to Contributors Needed in Good first issues Jan 10, 2024
@LucaTamSapienza
Copy link
Contributor

Hi, can i work on this issue?

@rghvsh
Copy link
Contributor

rghvsh commented Feb 3, 2024

.take

Copy link
Contributor

github-actions bot commented Feb 3, 2024

Thank you for looking into this issue! Please let us know if you have any questions or require any help.

@rghvsh
Copy link
Contributor

rghvsh commented Feb 3, 2024

hey! @LucaTamSapienza would be happy to work together if you want to

@LucaTamSapienza
Copy link
Contributor

@rghvsh sure! i've alredy implemented the expm1.cpp file and also the test for it. Just forgot to do .take to get the issue

Copy link
Contributor

github-actions bot commented Feb 3, 2024

Thanks for being interested in this issue. It looks like this ticket is already assigned to a contributor. Please communicate with the assigned contributor to confirm the status of the issue.

@rghvsh
Copy link
Contributor

rghvsh commented Feb 3, 2024

Ah! that's great let's ask one of the members to co-assign us and I'll get started

@LucaTamSapienza
Copy link
Contributor

@ilya-lavrenov

@LucaTamSapienza
Copy link
Contributor

@rghvsh if you have discord we can talk about it because rn i'm having some issues while i'm running my test

@rghvsh
Copy link
Contributor

rghvsh commented Feb 3, 2024

sure rzxcs is my username

@mlukasze mlukasze moved this from Contributors Needed to Assigned in Good first issues Feb 5, 2024
@p-wysocki
Copy link
Contributor

Hello, is there any help needed?

@LucaTamSapienza
Copy link
Contributor

Actually yes, I'm having a lot of issues due to precision FP::16,32. I've linked my PR above.

@mlukasze mlukasze moved this from Assigned to In Review in Good first issues Feb 27, 2024
github-merge-queue bot pushed a commit that referenced this issue Feb 28, 2024
### Details:
 *added aten::expm1 operation*
I'm having some problems during the tests; I think they're related to
the **frontend.cpp** file. My question is if I need to create a class to
support the expm1 operation and add it inside the **transform.cpp**
file? After that I would call it inside the frontend.cpp like this
`manager.register_pass<ov::frontend::pytorch::pass::Class_name>();`



### Tickets:
 - Closes #20578 

### Problem

E openvino._pyopenvino.OpConversionFailure: Check
'is_conversion_successful' failed at
src/frontends/pytorch/src/frontend.cpp:141:
E       FrontEnd API failed with OpConversionFailure:
E       Model wasn't fully converted.
E       Summary:
E       -- No conversion rule found for operations: aten::expm1

/opt/intel/openvino_2023.3.0/python/openvino/frontend/frontend.py:18:
OpConversionFailure

---------

Co-authored-by: Ekaterina Aidova <[email protected]>
@github-project-automation github-project-automation bot moved this from In Review to Closed in Good first issues Feb 28, 2024
@mlukasze mlukasze added this to the 2024.1 milestone Feb 28, 2024
@mlukasze
Copy link
Contributor

thank you (again!) @LucaTamSapienza :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: PyTorch FE OpenVINO PyTorch Frontend good first issue Good for newcomers no_stale Do not mark as stale
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants