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

refactoring drop mara-config integration #3

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

leo-schick
Copy link
Member

This PR drops dependency to mara_config by:

  • automatically importing all modules starting with mara-
  • using the same logic as in package mara-app to discover commands. The logic is moved into a internal submodule mara_cli._mara_modules
  • if no available command is found, a error is thrown suggesting to install package mara-pipelines via pip install mara-pipelines

@leo-schick
Copy link
Member Author

@jankatins I did now a new refactoring based on a entry point discovery of mara commands:

  1. Packages need to specify their commands with entrypoint group mara.commands. Examples: add mara.commands entrypoint mara-db#78 and add mara.commands entrypoint mara-pipelines#108
  2. no discovery is done anymore on MARA_CLICK_COMMANDS.
  3. since MARA_CLICK_COMMANDS are skipped, the old commands like flask mara_pipelines.ui.run are not available anymore. It is enforced to use the new style mara pipelines run

Example of mara --help

image

mara_cli/cli.py Outdated Show resolved Hide resolved
@jankatins
Copy link
Member

jankatins commented Dec 1, 2023

The code itself looks good, but for me the main question remains: how do you want to activate the rest of the functionality? Also via entry points? Or do that via a central "put it all together" file/function? Or keep it like now which looks into all imported stuff (Which after this PRs current implmentation would be defined by entry points, as every thing which adds a mara command now also gets imported, so whatever is in the import path will show up in a web cli and so on..

From my standpoint, it should be one way and not multiple:

  • everything via entry points: Nice standard, might be slow in some code path, e.g. the flask UI?), "install" = "activation", leads to split packages as one wants to make sure that everything which is optional now needs to go into another package to not get loaded automatically. Kind of implicit activation, so the developer of a package decides what gets activated and the user has to opt in for the whole package.
  • Everything via a central file but explicit activation: needs some kind of "activation" mechanism, e.g. via a central "register" method, awkward because of the strange central file/function which acts as an central entry point into the code; Needs a central registry; explicit, which has good and bad sides, one of the good sides is that the user decides what functionality gets activated.
  • Everything via a central file but implicit activation: "Import" = "activate", so similar to now. awkward because of the strange central file/function which acts as an central entry point into the code. Awkward because in regular python "import" is usually assumed of of "not changing the behavior of an app" and this assumption is broken by this (e.g. a lot of linter complain about "unused imports"). A bit more fine grained than a with the "entry point" mechanism as one can simply not import stuff (if that's possible). Does not need a central registry; implicit, which again has good and bad sides, one good side is that the developer decides that new functionality shows up and so no need for the "user" to change stuff for new functionality, update is enough.

My personal preference is "central file, but explicit activation", but I know that Martin was a firm believer into "It's nice that new functionality directly gets activated when a developer adds it to a package".

@jankatins
Copy link
Member

Another thing: now that mara cli is doing the entry point into the whole app, we have actually two: one for flask and one for mara-cli: is that intended or do you want to also make a mara run-flask (which now needs a pointer to the flask app)?

@leo-schick
Copy link
Member Author

My current idea of this package is to provide a easy-to-use CLI interface for mara. In this PR I did not want to make a decision (yet) how configuration should be done for mara. I think we can discuss this in #5.

My current design idea is the following:

Provide a way for modules to publish their CLI commands. The module developer decides which command to be added to entry point mara.commands. If a module does not supply click commands, it should mot provide entry point mara.commands.

I guess modules are not activated by the entry points. As I understand the code, we just load e.g. mara_db.cli, not mara_db. (I don't know if python automatically imports mara_db.__init__ if import mara_db.cli is called. I guess not). The module developer could decide to import mara_db inside mara_db.cli but that is out of the power of mara-cli.

I did not yet spend so much thoughts aobut the flask run integration but my thought was that a new entry point for mara.commands could be added by module mara-app to provide commands like mara run-flask, mara ui run or something similar...
In my environment, I plan to deprecate the mara flask UI - so, maybe someone else could take the lead in this.

@jankatins
Copy link
Member

jankatins commented Dec 4, 2023

No: any __init__.py in the path of the final to-be-imported

image

I would be a bit sad to not see any UI: it was a big part of my main dev env and made a lot of things a lot easier

In any case: I'm not using mara currently, so consider me a slightly more talkative rubber duck... :-)

@leo-schick
Copy link
Member Author

OK, so I was wrong with my assumtion ...

I think I will stick with the idea of Martin to just import the modules as soon as they are installed. When we run in the future into any issues with this, we can then add an environment variable which skips the auto-module-import logic and people can manually import the modules in their mara_config.py which will be auto loaded (see next upcomming commits).

@leo-schick leo-schick merged commit 16f4e89 into main Dec 6, 2023
5 checks passed
@leo-schick leo-schick deleted the refactor-drop-mara-config-integration branch December 6, 2023 17:08
@leo-schick
Copy link
Member Author

@jankatins p.s. I do not plan to drop the UI. It is charm! I just currently focus on CLI UX and try to make it more "less UI enfored" :-)

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.

2 participants