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

Find way for accessing SQlite DB programmatically #1690

Open
jotaen4tinypilot opened this issue Nov 22, 2023 · 1 comment
Open

Find way for accessing SQlite DB programmatically #1690

jotaen4tinypilot opened this issue Nov 22, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@jotaen4tinypilot
Copy link
Contributor

In #1524, we discussed that we should make it easier to use the SQlite / tinypilot.db file in programmatic contexts, e.g. in bash scripts.

Currently, you have to add a CLI endpoint to the Flask app (see, this example, where we read the streaming mode). This has some downsides:

  • Unless we come up with a more generic endpoint, we’d have to provide a CLI blueprint for each and every data point that we want to obtain from the SQlite.
  • Invoking Flask via the CLI is relatively slow: on device, it takes ~1.5 seconds for each invocation, where the majority of that time is spent initializing Flask.
  • Currently, Flask emits unrelated app-level logging to stdout, which makes the results hard to parse. This can probably be configured / disabled somehow, though.

Note that we can’t “naively” read from the tinypilot.db file as is, since the DB file might not be in the latest migration state. We probably don’t want to port our migration logic to a different language.

So we should explore options to access the tinypilot.db file in programmatic contexts. Requirements for the solution:

  • Easy to use, with minimal boilerplate
  • Safe – e.g., don’t bypass the migration mechanism
  • Performant – like, well below 1 second

Potential solutions / approaches:

(Non-exhaustive, just as starting points)

  • We could isolate the app.db module in a way that we can write our own, Flask-less CLI entry point.
  • We investigate why Flask is so slow, and see how we can make it faster for this purpose.
@jotaen4tinypilot
Copy link
Contributor Author

jotaen4tinypilot commented Nov 22, 2023

Re the idea of isolating the app.db module and providing our own, Flask-less CLI entry point:

Out of curiosity, I ran a quick-and-dirty POC on this branch. With this, you could do the following on the command line:

$ ./cli settings streaming-mode
MJPEG
$ ./cli settings requires-https
True
$ ./cli users names
['John Doe']

For that, we’d have to refactor the db_connection handling, and effectively make the app.db module to have no upwards dependencies, otherwise we’d unintendedly loop in Flask.

Execution time on device (including activating venv) is ~120ms, which seems reasonable (well, at least more reasonable than 1500ms).

jotaen4tinypilot added a commit that referenced this issue Nov 30, 2023
Resolves #1524.

The outcome of our discussion in the ticket was this:

- The SQlite DB (`tinypilot.db`) should be the default place for new
settings, whereas the YAML file (`settings.yml`) should only be used in
exceptional cases, and obviously for legacy reasons.
- We favor having a single storage solution over a dual approach, as
that simplifies future decision-making, and eliminates potential
migration burden between the two storage formats.
- The SQlite DB doesn’t easily allow programmatic access right now (in
contrast to the YAML file), so we [created this ticket for removing this
hurdle](#1690).
- We [will document the parameters in `settings.yml`
separately](#1573).

This PR adds a comment in `settings.py`, marking its usage as
quasi-deprecated. It also adds some historical context, references the
SQlite DB as default storage, and lists sample exceptional cases.

I realized that we probably don’t need to add any documentation to the
SQlite DB, since there is not really an alternative to it any longer. So
by deprecating `settings.yml`, the SQlite DB is the only storage option
anyway, and there shouldn’t be any ambiguity left. Contrary to [my
initial
proposal](#1524 (comment)),
I also don’t think we should move `settings.py` up to the `app` package,
as that might mistakenly promote its usage.
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1692"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

---------

Co-authored-by: Jan Heuermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant