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

added instagram channel code #9594

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

Horizon733
Copy link

@Horizon733 Horizon733 commented Sep 9, 2021

Proposed changes:

Please let me know what all changes are to be done since this is my first PR.

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@Horizon733
Copy link
Author

I added changelog, need help with code quality check, how to run tests and integration tests

@TyDunn
Copy link
Contributor

TyDunn commented Sep 10, 2021

Sweet! I added the other issue it addresses to the description and asked an engineer to give you some guidance. Please make sure to target Rasa Open Source 3.0 since this is an enhancement (check out the repo README for more information)

@Horizon733
Copy link
Author

Horizon733 commented Sep 10, 2021

Sweet! I added the other issue it addresses to the description and asked an engineer to give you some guidance. Please make sure to target Rasa Open Source 3.0 since this is an enhancement (check out the repo README for more information)

Sure will focus on 3.0 but how do I test this with 3.0?

@Horizon733 Horizon733 marked this pull request as ready for review September 15, 2021 10:49
@Horizon733 Horizon733 requested a review from a team as a code owner September 15, 2021 10:49
@Horizon733 Horizon733 requested review from alwx and removed request for a team September 15, 2021 10:49
@Horizon733
Copy link
Author

Sweet! I added the other issue it addresses to the description and asked an engineer to give you some guidance. Please make sure to target Rasa Open Source 3.0 since this is an enhancement (check out the repo README for more information)

Any update?

@TyDunn
Copy link
Contributor

TyDunn commented Sep 20, 2021

Any update?
Yes. Sorry for the delay. We have been really busy the last couple weeks. It looks like an engineer will take a look at this this week

Copy link
Contributor

@alwx alwx left a comment

Choose a reason for hiding this comment

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

Hey @Horizon733 and thanks for your contribution!

I did a review and have a couple of questions for you:

  1. It looks very very similar to the code in rasa/core/channels/facebook.py — I guess it's because Instagram uses Messenger as well. Can we somehow use the same Messenger class that we already have for Instagram conversations or at least write some generic class for both Instagram and Facebook?
  2. Will it work for non-Messenger conversations? The Messenger update for Instagram seems to only be available for those who have their Instagram account linked to their Facebook account (https://about.fb.com/news/2020/09/new-messaging-features-for-instagram/); the rest use standard Instagram messaging.
  3. Have you considered adding tests for this channel?

rasa/core/channels/instagram.py Outdated Show resolved Hide resolved
rasa/core/channels/instagram.py Outdated Show resolved Hide resolved
rasa/core/channels/instagram.py Outdated Show resolved Hide resolved
@Horizon733
Copy link
Author

Hey @Horizon733 and thanks for your contribution!

I did a review and have a couple of questions for you:

  1. It looks very very similar to the code in rasa/core/channels/facebook.py — I guess it's because Instagram uses Messenger as well. Can we somehow use the same Messenger class that we already have for Instagram conversations or at least write some generic class for both Instagram and Facebook?
  2. Will it work for non-Messenger conversations? The Messenger update for Instagram seems to only be available for those who have their Instagram account linked to their Facebook account (https://about.fb.com/news/2020/09/new-messaging-features-for-instagram/); the rest use standard Instagram messaging.
  3. Have you considered adding tests for this channel?
  1. yes it does look similar, we can make both of them generic so we can use only one for Instagram and Facebook. I did try that and got success since I just had to add the credentials in the Instagram section but we have to be careful like Instagram is in Beta so, till then we can keep both separate and once we are confirmed they will be using the same formats then we can go ahead and merge them into one file
  2. I wasn't able to properly understand, if possible could you please elaborate?
  3. I don't know how to write tests, I would really appreciate it if you helped me out, since it is my first code contribution

@alwx
Copy link
Contributor

alwx commented Sep 23, 2021

@Horizon733

To be honest, I don't think it's worth adding this class in the current form — the rasa/core/channels/instagram.py is basically just a duplicate for rasa/core/channels/facebook.py, and probably the right way of doing it is to just update the documentation and indicate that the existing Messenger class needs to be used for Instagram since Instagram uses Messenger anyway.
I got your point the Messenger in Instagram is still in beta but I think this aspect should be covered by tests.

Would be happy to hear more opinions on that. @TyDunn, @wochinge?

@TyDunn
Copy link
Contributor

TyDunn commented Sep 23, 2021

@alwx I did not realize that the APIs were so integrated across Instagram and Facebook. If is just a matter of updating the docs and making slight changes to the existing Messenger channel (e.g. maybe changing the name from facebook.py to messenger.py) to make it clear and easy for users to use it with Instagram, then I agree that we should go that route

@Horizon733
Copy link
Author

@alwx I did not realize that the APIs were so integrated across Instagram and Facebook. If is just a matter of updating the docs and making slight changes to the existing Messenger channel (e.g. maybe changing the name from facebook.py to messenger.py) to make it clear and easy for users to use it with Instagram, then I agree that we should go that route

Yes, we can definitely do that, and add some checks for unique messages of facebook and Instagram. Let me know if anything is there, I could help out in changing facebook.py to messenger.py to keep it generic

@Horizon733
Copy link
Author

@alwx I did not realize that the APIs were so integrated across Instagram and Facebook. If is just a matter of updating the docs and making slight changes to the existing Messenger channel (e.g. maybe changing the name from facebook.py to messenger.py) to make it clear and easy for users to use it with Instagram, then I agree that we should go that route

Quick update: I check recently and Instagram is supporting generic template aka carousels. So, if we are planning to make a generic channel for facebook and Instagram. I am up for it

@alwx
Copy link
Contributor

alwx commented Oct 20, 2021

@Horizon733 that would be much better. If you're interested in working on it then sure, you can go for it (and I can help you with that if you need any help).

And sorry for the late response — was a bit busy recently.

@Horizon733
Copy link
Author

@Horizon733 that would be much better. If you're interested in working on it then sure, you can go for it (and I can help you with that if you need any help).

And sorry for the late response — was a bit busy recently.

Sure, will start, please let me know anything that I have to keep in mind before starting to code.

Copy link
Contributor

@rgstephens rgstephens left a comment

Choose a reason for hiding this comment

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

Should also add a page to the docs. Probably similar to existing Facebook connector page

@Horizon733
Copy link
Author

Hi @TyDunn @alwx Happy new year. I have merged and solved conflicts with main branch. Please review and let me know if there is anything to be done or changed

@Horizon733
Copy link
Author

Also, I would like to tell you guys, I am not able to run livedocs since it gives me following error. I did check googling it was showing there might be path issue with windows machine.

(env) D:\rasa>make livedocs             
cd docs/ && poetry run yarn start

  FileNotFoundError

  [WinError 2] The system cannot find the file specified

  at E:\Anaconda3\lib\subprocess.py:1207 in _execute_child
      1203│                                          int(not close_fds),
      1204│                                          creationflags,
      1205│                                          env,
      1206│                                          os.fspath(cwd) if cwd is not None else None,
    → 1207│                                          startupinfo)
      1208│             finally:
      1209│                 # Child is launched. Close the parent's copy of those pipe
      1210│                 # handles that only the child should have open.  You need
      1211│                 # to make sure that no handles to the write end of the
Makefile:233: recipe for target 'livedocs' failed
make: *** [livedocs] Error 1
```

@TyDunn TyDunn removed the request for review from alwx January 10, 2022 05:50
@losterloh
Copy link
Contributor

@Horizon733 Are you still interested in getting this reviewed & merged? 🙂 If yes, please take care of the conflicts and then we're happy to take another look!

@Horizon733
Copy link
Author

@Horizon733 Are you still interested in getting this reviewed & merged? 🙂 If yes, please take care of the conflicts and then we're happy to take another look!

Hi I am interested in merging and having this feature, but the reviewing and approval takes so long so this conflicts occur if see all merges above i have solve conflicts many times. I will do solve conflicts if it will be reviewed fast and provide me review. Because as Rasa is this big code are changes everytime in week.

@losterloh
Copy link
Contributor

@Horizon733 Yes, we will be happy to do a fast review!

@Horizon733
Copy link
Author

@Horizon733 Yes, we will be happy to do a fast review!

ok will fix all conflcts by Friday max. Thanks

…ram-integration

� Conflicts:
�	.pre-commit-config.yaml
�	docs/docs/connectors/facebook-and-instagram-messenger.mdx
�	rasa/nlu/emulators/dialogflow.py
�	rasa/nlu/extractors/entity_synonyms.py
�	rasa/nlu/featurizers/sparse_featurizer/count_vectors_featurizer.py
�	rasa/shared/core/generator.py
�	rasa/shared/core/slots.py
�	rasa/shared/core/trackers.py
�	rasa/shared/core/training_data/story_writer/yaml_story_writer.py
�	rasa/shared/core/training_data/structures.py
�	tests/core/evaluation/test_marker.py
�	tests/core/evaluation/test_marker_stats.py
�	tests/core/featurizers/test_single_state_featurizers.py
�	tests/core/test_processor.py
�	tests/graph_components/validators/test_default_recipe_validator.py
�	tests/nlu/classifiers/test_diet_classifier.py
�	tests/nlu/featurizers/test_lm_featurizer.py
�	tests/nlu/selectors/test_selectors.py
�	tests/nlu/test_train.py
�	tests/utils/tensorflow/test_models.py
�	tests/utils/tensorflow/test_rasa_layers.py
@Horizon733
Copy link
Author

@Horizon733 Yes, we will be happy to do a fast review!

@losterloh Can you please review the code and let me know

@sanchariGr
Copy link
Collaborator

Hey @Horizon733, your changes look good but would be great if you can add some tests along with it, as a reference we have the existing channel tests for facebook or any of the other supported channels can be found here. Feel free to take a look and let us know how you feel about this and if you need some more infos.

@Horizon733
Copy link
Author

Hey @Horizon733, your changes look good but would be great if you can add some tests along with it, as a reference we have the existing channel tests for facebook or any of the other supported channels can be found here. Feel free to take a look and let us know how you feel about this and if you need some more infos.

Hi @sanchariGr Sorry for late reply, I am writing tests, I will definitely try to push it by this week's end.

@Horizon733
Copy link
Author

Hi @TyDunn @sanchariGr I am facing below error because of pre-commit and black can you guys please help me out committing and pushing changes so we can move ahead?

(.venv) F:\rasa>git commit -m "tests for messenger added"
black....................................................................Failed
- hook id: black
- exit code: 1

Traceback (most recent call last):
  File "D:\miniconda3\envs\rasa\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "D:\miniconda3\envs\rasa\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\Scripts\black.EXE\__main__.py", line 7, in <module>
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\black\__init__.py", line 1129, in patched_main
    patch_click()
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\black\__init__.py", line 1115, in patch_click
    from click import _unicodefun  # type: ignore
ImportError: cannot import name '_unicodefun' from 'click' (C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\click\__init__.py)

doctoc...............................................(no files to check)Skipped
docstring-check..........................................................Failed
- hook id: docstring-check
- exit code: 2

git diff main...HEAD -- rasa | poetry run flake8 --select D --diff
rasa/core/channels/messenger.py:25:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:28:1: D107 Missing docstring in __init__
rasa/core/channels/messenger.py:175:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:178:1: D107 Missing docstring in __init__
rasa/core/channels/messenger.py:187:1: D202 No blank lines allowed after function docstring
rasa/core/channels/messenger.py:294:1: D202 No blank lines allowed after function docstring
rasa/core/channels/messenger.py:333:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:337:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:360:1: D417 Missing argument descriptions in the docstring
rasa/core/channels/messenger.py:378:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:452:1: D102 Missing docstring in public method
make: *** [Makefile:95: lint-docstrings] Error 1

The docstrings are similar to which we have in facebook.py channel code not any addon or any removal just typos have been changed then too it is cancelling commits

@sanchariGr
Copy link
Collaborator

Hi @TyDunn @sanchariGr I am facing below error because of pre-commit and black can you guys please help me out committing and pushing changes so we can move ahead?

(.venv) F:\rasa>git commit -m "tests for messenger added"
black....................................................................Failed
- hook id: black
- exit code: 1

Traceback (most recent call last):
  File "D:\miniconda3\envs\rasa\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "D:\miniconda3\envs\rasa\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\Scripts\black.EXE\__main__.py", line 7, in <module>
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\black\__init__.py", line 1129, in patched_main
    patch_click()
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\black\__init__.py", line 1115, in patch_click
    from click import _unicodefun  # type: ignore
ImportError: cannot import name '_unicodefun' from 'click' (C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\click\__init__.py)

doctoc...............................................(no files to check)Skipped
docstring-check..........................................................Failed
- hook id: docstring-check
- exit code: 2

git diff main...HEAD -- rasa | poetry run flake8 --select D --diff
rasa/core/channels/messenger.py:25:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:28:1: D107 Missing docstring in __init__
rasa/core/channels/messenger.py:175:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:178:1: D107 Missing docstring in __init__
rasa/core/channels/messenger.py:187:1: D202 No blank lines allowed after function docstring
rasa/core/channels/messenger.py:294:1: D202 No blank lines allowed after function docstring
rasa/core/channels/messenger.py:333:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:337:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:360:1: D417 Missing argument descriptions in the docstring
rasa/core/channels/messenger.py:378:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:452:1: D102 Missing docstring in public method
make: *** [Makefile:95: lint-docstrings] Error 1

The docstrings are similar to which we have in facebook.py channel code not any addon or any removal just typos have been changed then too it is cancelling commits

Hey @Horizon733 looks to me like both flake8 and black are flagging some linting issues. Can you try fixing the issues you get after running poetry run flake8 rasa tests --extend-ignore D and poetry run black rasa tests and then try to commit.

@Horizon733
Copy link
Author

Hi @TyDunn @sanchariGr I am facing below error because of pre-commit and black can you guys please help me out committing and pushing changes so we can move ahead?

(.venv) F:\rasa>git commit -m "tests for messenger added"
black....................................................................Failed
- hook id: black
- exit code: 1

Traceback (most recent call last):
  File "D:\miniconda3\envs\rasa\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "D:\miniconda3\envs\rasa\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\Scripts\black.EXE\__main__.py", line 7, in <module>
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\black\__init__.py", line 1129, in patched_main
    patch_click()
  File "C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\black\__init__.py", line 1115, in patch_click
    from click import _unicodefun  # type: ignore
ImportError: cannot import name '_unicodefun' from 'click' (C:\Users\disha\.cache\pre-commit\repowotxool4\py_env-python3\lib\site-packages\click\__init__.py)

doctoc...............................................(no files to check)Skipped
docstring-check..........................................................Failed
- hook id: docstring-check
- exit code: 2

git diff main...HEAD -- rasa | poetry run flake8 --select D --diff
rasa/core/channels/messenger.py:25:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:28:1: D107 Missing docstring in __init__
rasa/core/channels/messenger.py:175:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:178:1: D107 Missing docstring in __init__
rasa/core/channels/messenger.py:187:1: D202 No blank lines allowed after function docstring
rasa/core/channels/messenger.py:294:1: D202 No blank lines allowed after function docstring
rasa/core/channels/messenger.py:333:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:337:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:360:1: D417 Missing argument descriptions in the docstring
rasa/core/channels/messenger.py:378:1: D102 Missing docstring in public method
rasa/core/channels/messenger.py:452:1: D102 Missing docstring in public method
make: *** [Makefile:95: lint-docstrings] Error 1

The docstrings are similar to which we have in facebook.py channel code not any addon or any removal just typos have been changed then too it is cancelling commits

Hey @Horizon733 looks to me like both flake8 and black are flagging some linting issues. Can you try fixing the issues you get after running poetry run flake8 rasa tests --extend-ignore D and poetry run black rasa tests and then try to commit.

Hi @sanchariGr,
It seems commit happens from linux or mac os without M1 chip and has difficulties with Windows. I hope you guys can have some tips on how to run tests on windows and able to commit via windows.
I have added some updates inside Docs please do check them and let me know if there needs to be any changes, we can have discussion like what heading, subheading and content we can have for docs.
Apart from that, tests has been added, please do check those.
Thanks

@sanchariGr
Copy link
Collaborator

@Horizon733 you might be facing this issue because your local version of black doesnot match the one on the CI, can you please try running make formatter and make lint?

@Horizon733
Copy link
Author

@Horizon733 you might be facing this issue because your local version of black doesnot match the one on the CI, can you please try running make formatter and make lint?

let me try and push the code

@Horizon733
Copy link
Author

image
Hi @sanchariGr, the lines that lint specifies were/are already like that, (missing docstrings and have blank lines) so is there any way to ignore those, or what can we do? I am pushing the reformatted code, please try and let me know how it is on your end

Copy link
Collaborator

@sanchariGr sanchariGr left a comment

Choose a reason for hiding this comment

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

@Horizon733 Unfortunately, the linter will flag any issues in the module that’s being edited, even though that particular code might not have been touched. The only way forward is for you to add docstrings. I have added some to help get started.

@@ -23,15 +23,17 @@ class Messenger:

@classmethod
def name(cls) -> Text:
return "facebook"
return "messenger"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return "messenger"
"""Return channel name."""
return "messenger"

@@ -170,11 +173,14 @@ class MessengerBot(OutputChannel):

@classmethod
def name(cls) -> Text:
return "facebook"
return "messenger"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return "messenger"
"""Return channel name."""
return "messenger"

def __init__(self, messenger_client: MessengerClient) -> None:
def __init__(
self, messenger_client: MessengerClient, messenger_service: Text
) -> None:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"""Initialises input channel."""

@sanchariGr
Copy link
Collaborator

Also @Horizon733 can you also run all the unit tests locally with make test there are a couple other tests that were dependant on facebook.py that are currently failing, you probably just need to change it to the updated name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for instagram as a channel
9 participants