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 config wizard #125

Merged
merged 6 commits into from
Oct 30, 2023
Merged

refactor config wizard #125

merged 6 commits into from
Oct 30, 2023

Conversation

pmeier
Copy link
Member

@pmeier pmeier commented Oct 27, 2023

This is my proposed solution for #124. It implements the following changes:

  • Remove the demo and builtin special configuration options. Note that the previous Config.demo() is still available, since that just created a Config() with default values anyway:

    ragna/ragna/core/_config.py

    Lines 123 to 125 in a04efc2

    @classmethod
    def demo(cls) -> ragna.Config:
    return cls()
  • Remove the default value for -c / --config. ragna worker, ragna api, and ragna ui now require passing a configuration.
  • Split ragna config and ragna config --check into ragna init and ragna check. ragna init is the configuration wizard and ragna check can be with configuration files to check availability of the requirements.
  • Improve the wizard by printing a summary of the unmet requirements as well as instructions on how to meet them.
  • A few phrasing improvements in the wizard.

@pmeier
Copy link
Member Author

pmeier commented Oct 29, 2023

One more change after an offline chat with @dharhas:

Instead of asking for confirmation after every single component that has unmet requirements, just assume that is what the user wants, since they have selected this in the step before. Since this PR also adds a summary of all the required packages and environment variables at the end, no information is lost through this.

We also discussed to have a "I just want everything that ragna has to offer" option, but I decided against it for now. Due to the change above, the selection is now only two questions. Coupled with the fact that you can toggle all options by pressing the a key when the choices are visible, IMO, we would make the process more complicated by adding another question. LMK if you feel different.

@smeragoel
Copy link

Agreed, simpler is better. Two options should be fine, and instead of over-optimising now, we can wait for user feedback to iterate.

@pmeier
Copy link
Member Author

pmeier commented Oct 30, 2023

One more final adjustment after another offline chat with @dharhas:

We now use the config file located at ./ragna.toml as default in case it exists. Meaning, if someone generates a config with ragna init, all the other commands work without explicitly setting the -c / --config option.

@pmeier
Copy link
Member Author

pmeier commented Oct 30, 2023

Doc error is unrelated:

ERROR - mkdocstrings: Couldn't load inventory {'url': 'https://fastapi.tiangolo.com/objects.inv'} through PythonHandler.load_inventory: HTTP Error 500: Internal Server Error

The FastAPI site is down.

@pmeier pmeier merged commit d4ee0ee into main Oct 30, 2023
9 of 10 checks passed
@pmeier pmeier deleted the force-config branch October 30, 2023 20:30
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.

None yet

2 participants