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

Bots: Add support for running packaged bots via entrypoints #708

Merged
merged 6 commits into from
Jul 29, 2021

Conversation

PIG208
Copy link
Member

@PIG208 PIG208 commented Jul 20, 2021

This revives #517;

TODO:

  • Consider using importlib.metadata or importlib_metadata instead of entrypoints (which doesn't seem to be actively maintained)
  • Add a packaged bot boilerplate for demonstration purpose
  • Add documentation for this feature

PIG208 added 2 commits July 22, 2021 11:14
The previous implementation to locate the `bot_dir` is unfortunately
wrong as it doesn't work with the external custom bots.
…paths.

This removes the need to have `load_module_from_file`.
@PIG208 PIG208 force-pushed the bot-dist branch 8 times, most recently from 6435a64 to ad578c0 Compare July 22, 2021 08:02
requirements.txt Outdated Show resolved Hide resolved
zulip_botserver/tests/test_server.py Outdated Show resolved Hide resolved
zulip_bots/setup.py Outdated Show resolved Hide resolved
@PIG208 PIG208 force-pushed the bot-dist branch 2 times, most recently from 0dfc2c3 to 11f29b2 Compare July 22, 2021 14:10
@PIG208 PIG208 marked this pull request as ready for review July 22, 2021 14:12
Copy link
Member

@eeshangarg eeshangarg left a comment

Choose a reason for hiding this comment

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

@PIG208 Thanks so much for working on this! This PR is looking good, I left a few minor comments/suggestions. As always, if you have any questions, please feel free to reach out. Cheers!

packaged_helloworld/README.md Outdated Show resolved Hide resolved
"packages": find_packages(),
}

setup(**package_info)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. have we thought about if these will be installed alongside the zulip_bots PyPI package or not? @timabbott My guess is we should keep these as separate packages? Although, that adds more release work to our process.

Copy link
Member

Choose a reason for hiding this comment

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

Regardless, we need to improve our automated release process if we are going to do this. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it won't be necessary. Since this package is only for demonstration and zulip_bots hardly rely on it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it's probably fine to have this be in the same repository for now. When we write documentation for this, I think what we'll actually want is a zulip-new-bot bot_name command that creates a directory of this form for you, so that we can have really simple steps for making a new bot.

zulip_bots/zulip_bots/finder.py Show resolved Hide resolved
zulip_bots/zulip_bots/run.py Outdated Show resolved Hide resolved
print(dep_err_msg.format(bot_name=bot_name, deps_list=deps_list))
bot_source, lib_module = finder.import_module_from_zulip_bot_registry(args.bot)
except finder.DuplicateRegisteredBotName:
print("ERROR: Found duplicate entries for bot name in zulip bot registry. Exiting now.")
Copy link
Member

Choose a reason for hiding this comment

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

We might want to give more information here as to how to resolve the error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I think displaying the conflicting entrypoint should be fairly sufficient.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that's right.

zulip_botserver/zulip_botserver/server.py Outdated Show resolved Hide resolved
PIG208 added 4 commits July 29, 2021 11:08
Now we will be able to execute `zulip-run-bot` with the `-r` argument
to search for and run bots from the `zulip_bots.registry` entry_point.

Each entry point should have the name correspond to the bot name,
and have the value be the bot module. E.g, an Python package for a
bot called "packaged_bot" should have an `entry_points` setup like
the following:

setup(
    ...
    entry_points={
        "zulip_bot.registry":[
            "packaged_bot=packaged_bot.packaged_bot"
        ]
    }
    ...
)

whose file structure may look like this:

packaged_bot/
├───packaged_bot/
|   ├───packaged_bot.py  # The bot module
|   ├───test_packaged_bot.py
|   ├───packaged_bot.conf
|   └───doc.md
└───setup.py  # Register the entry points here

Add test case.
Add packaged_helloworld as an example of a PyPI package setup for
an external zulip bot that can be installed via pip and lanuched
without the need to include it in the zulip_bots/bots directory.
@timabbott timabbott merged commit 285a946 into zulip:master Jul 29, 2021
@timabbott
Copy link
Member

I fixed a typo in one of the docstrings, and merged this. I think we will probably want to iterate further on the concept, but the best way to do this is to push on the documentation for how to create your own bot -- ideally we want it to be really simple and not involve manual copy-paste work. I posted an idea above that we could have a tool generate a template bot for you (that you can then git init or just run pip install . to get started); that would mean you don't really need to think.

The other option is that we offer a GitHub repository for you to fork; but the problem with that is that you'll now have a repository for example_bot, which isn't what you wanted, so your first step is to do a bunch of renaming.

@neiljp
Copy link
Contributor

neiljp commented Jul 29, 2021

It's great to see this merged! Thanks @PIG208 ! 🎉

PIG208 added a commit to PIG208/python-zulip-api that referenced this pull request Jul 30, 2021
Following support to running bots from entry points in zulip#708, we
implement this `create-zulip-bot` tool to simplify the process of
creating new bots. The user will be able to directly install the package
with pip and run the bot with `zulip-run-bot`, or use it to quickly set
up a git repository.

Note that the boilerplate generated by this script does not contain
`tests.py` yet. We need to figure out the right pattern for integrating
unittests for such packaged bots.
PIG208 added a commit to PIG208/python-zulip-api that referenced this pull request Aug 2, 2021
Following support to running bots from entry points in zulip#708, we
implement this `create-zulip-bot` tool to simplify the process of
creating new bots. The user will be able to directly install the package
with pip and run the bot with `zulip-run-bot`, or use it to quickly set
up a git repository.

Note that the boilerplate generated by this script does not contain
`tests.py` yet. We need to figure out the right pattern for integrating
unittests for such packaged bots.
@PIG208 PIG208 deleted the bot-dist branch August 20, 2021 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants