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

Make server more flexible #576

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

dsblank
Copy link
Member

@dsblank dsblank commented Nov 21, 2024

This PR does the following:

  1. Refactors the logging so there is only one (the app logger)
  2. Adds flags to allow more flexibility
  • --use-wsgi to add a wsgi wrapper
  • --host for wsgi listener
  • --debug-level set the log level
  • --log-file if you want to log to file
  • --open open the browser on host:port
  • --max-workers for wsgi server
  1. The default flag values should allow server to run exactly as before (although the log format is now more standard and colorized)

@DavidMStraub
Copy link
Member

Thanks, good ideas! The only problem I see is that if somebody pip-installs Gramps Web API and uses the CLI to run it with the new --browser option, it will simply open an empty page in the browser. I see two options to improve on this:

  1. Check if {STATIC_PATH}/index.html exists and is non-empty. If not, issue a warning that for running Gramps Web, downloading the frontend is required, and refuse opening the browser.
  2. Replace static/index.html shipped with the Python package by a placeholder file containing something like
<h1>Welcome to Gramps Web API!</h1>
<p>If you see this page, Gramps Web API is up and running.</p>
<p>Gramps Web API provides a REST API to work with a Gramps family tree database.
It also servers as the backend of Gramps Web, an integrated web app.
To run Gramps Web, <a href="https://github.com/gramps-project/gramps-web/releases">download
the frontend</a> and set the Gramps Web API `STATIC_PATH` configuration variable.</p>

What do you think?

max_workers = min(32, os.cpu_count() + 4)

def open_webbrowser():
time.sleep(1.0)
Copy link
Member

Choose a reason for hiding this comment

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

why sleep?

Copy link
Member Author

Choose a reason for hiding this comment

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

That gives the server time to start. If you open the page too quickly, you'll get an error.

app.logger.addHandler(file_handler)
app.logger.setLevel(debug_level)

print("Running gramps-web...")
Copy link
Member

Choose a reason for hiding this comment

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

Gramps Web API

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 debated that. On one hand, the gramps-web-api is just the REST server. Thinking of suggesting renaming gramps-web to gramps-web-frontend, plus gramps-web-api together as gramps-web (and I'm thinking about a package just called gramps-web). What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Thinking of suggesting renaming gramps-web to gramps-web-frontend, plus gramps-web-api together as gramps-web

That's exactly the naming we have. The frontend used to have a separate name (Gramps.js, "the Gramps Web frontend"), but we renamend the repository to gramps-web because it lead to confusion.

From the point of view of the combined web application, it would make sense to have a monorepo and combined packaging, but so far we still have them separated. Since the API can be used for other things as well, I think it kind of makes sense. And for proper web-based deployments, it's anyway more complicated (celery, nginx, etc.).

What we could do is to add a convenience command to download a frontend.

import urllib.parse


class TransLogger(object):
Copy link
Member

Choose a reason for hiding this comment

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

(object) is not needed (I know this is copy-pasted from somewhere else, but we can modernize a bit)

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 can fix a couple of other things there too. Now that I know you are open to this PR, I will do that.


def open_webbrowser():
time.sleep(1.0)
new = {"tab": 0, "window": 1}[open]
Copy link
Member

Choose a reason for hiding this comment

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

This will raise a KeyError if the user provides any unexpected string. I think it would be better to use a click choice option for the browser flag.

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'll take a look.

@DavidMStraub
Copy link
Member

One thing I'm unsure about: which things will break when using threads. So far, Web API was either served by the flask development server (which is single-threaded) or by gunicorn using worker processes (not threads). Did you investigate that already?

@dsblank
Copy link
Member Author

dsblank commented Nov 23, 2024

Did you investigate that already?

I assume that we will not run those two paths with the other flags. I'm developing a third flow that runs the server + has frontend preinstalled and runs locally.

@dsblank
Copy link
Member Author

dsblank commented Nov 23, 2024

First, thanks for being open to changes!

Regarding the:

Check if {STATIC_PATH}/index.html exists and is non-empty. If not, issue a warning that for running Gramps Web, downloading the frontend is required, and refuse opening the browser.

I think that is good in general. Currently, one doesn't know if the frontend is available or not, right? It would be good to show something like:

Warning: the gramps-web-frontend was not found. Starting gramps-web-api as just
a server.

@dsblank
Copy link
Member Author

dsblank commented Nov 23, 2024

Does it ever make sense to run the gramps-web-api server without a frontend? I guess for testing. Anything else? Maybe that is just an error, without something like --server-only?

@DavidMStraub
Copy link
Member

Does it ever make sense to run the gramps-web-api server without a frontend? I guess for testing. Anything else? Maybe that is just an error, without something like --server-only?

In principle, it could serve as backend of various different apps. Somebody could develop some tailored frontends, say with some dedicated forms for entering census data; or for native mobile apps. That was the original idea when we started the API without a frontend.

It can also be used as a way to do batch operations on a Gramps tree without using Python code, just HTTP request.

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