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

Refactor mara flask CLI commands #42

Open
leo-schick opened this issue Jul 2, 2022 · 3 comments
Open

Refactor mara flask CLI commands #42

leo-schick opened this issue Jul 2, 2022 · 3 comments
Labels

Comments

@leo-schick
Copy link
Member

The cli commands returned by the function MARA_CLICK_COMMANDS are automatically added to the Mara Flask App (code see here).

There is a default way in Flask to add extra commands from a module by using a entry_point in the module package with name flask.commands: [...]. See here. This functionality is already available since 1.0.0 (pallets/flask#2259)

I suggest to deprecate the current pattern of using the MARA_CLICK_COMMANDS function in advance to the flask standard way of dealing with this.

In addition, I suggest to redesign the command names / and keep the old names only for upgrade compatibility. The command flask mara_pipelines.ui.run could be e.g. changed into <my_mara_app> pipeline run by using a click group.

@leo-schick
Copy link
Member Author

leo-schick commented Feb 7, 2023

I did a bit discovering in the code and came on the folllowing idea:

When the MARA_CLICK_COMMANDS function returns a click.Command instance, it should behave as of today: The command is made available as <package_name>.<command_name> e.g. flask mara_pipelines.ui.run.

When the MARA_CLICK_COMMANDS function retunrs a click.MultiCommand instance or a derivate of it, it should pass over the multi command itself. With that it would be possible smoothly to integrate a new logic.

For example:

import click

@click.group
def pipeline():
    pass

@pipeline.command
def run(...):
    # ...
    pass

MARA_CLICK_COMMANDS = [pipeline]

would then result in the following CLI command:

flask pipeline run

Using a custom script named mara would then result in the following call:

mara pipeline run

@jankatins
Copy link
Member

I had similar ideas a few years ago (https://github.com/jankatins/mara-cli + https://github.com/jankatins/mara-config/) but entry points were quite slow at that point. Not sure if that got better since.

@leo-schick
Copy link
Member Author

leo-schick commented Feb 7, 2023

Maybe I don't get your point here about the entry points ... I never experienced performance issues with entrypoints. I see people using it in all sort of projects

About the config (mara-config): I see that that is something which we should tackle. I really like the idea how superset does it: It has a superset_config.py file which looks like a .env, but it allows you additionally to use custom python code in it. The file is imported via a flask app core function which takes care of the rest. I don't see the big benefit with this config patching but maybe there is something I miss here...

Another thing I have in mind is to replace mara-acl with Flask-AppBuilder. It is just too much for the mara project to develop all this logic with Authentication, Authorization, user groups management etc. I would rather use an external tool for this (which btw. adds the cli command group flask fab ... to let you manage user accounts from the command line).

leo-schick added a commit that referenced this issue Feb 14, 2023
* do not modify the click command name when it is a MultiCommand #42

* add patchable function for register_command
@leo-schick leo-schick modified the milestones: 3.0.0, 2.4.0 Feb 21, 2023
leo-schick added a commit that referenced this issue Feb 23, 2023
* do not modify the click command name when it is a MultiCommand #42

* add patchable function for register_command
@leo-schick leo-schick removed this from the 2.4.0 milestone May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants