support to launch Aqua services without jupyterlab #1075
Conversation
mayoor
requested review from
darenr,
mrDzurb,
VipulMascarenhas,
qiuosier and
ahosler
as code owners
oracle-contributor-agreement
bot
added
the
OCA Verified
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
|
|
📌 Cov diff with main: 📌 Overall coverage: |
|
📌 Cov diff with main: 📌 Overall coverage: |
| logger = getLogger(__name__) | ||
| config_location = os.path.join(os.getcwd(), ".env") | ||
| if os.path.exists(config_location): | ||
| logger.info(f"Loading environment variables from {config_location}") |
There was a problem hiding this comment.
NIT:
if load_dotenv(dotenv_path=config_location):
logger.info(f"Environment variables successfully loaded from {config_location}.")
Do we want to use this option as well?
verbose: Whether to output a warning the .env file is missing.
There was a problem hiding this comment.
I have changed the else part to warning
Are you suggesting that we log based on the verbose option? By default we dont show anything?
There was a problem hiding this comment.
I meant, we could show the success message if any variables were loaded, but then I thought it would be useful to know which variables were not loaded and verbose flag i guess can give us such info? If we do this we don't need to show the logger.info(f"Loading environment variables from {config_location}").
But it was marked as NIT in any case :) we can leave it as is.
ads/aqua/server/__main__.py
Outdated
| logger.info("Environment variables loaded successfully") | ||
| else: | ||
| logger.info( | ||
| f"{config_location} not found. Conside using `.env` file to setup defalut environment variables" |
There was a problem hiding this comment.
Typo?
Conside -> Consider
defalut -> default
logger.info(
f"{config_location} not found. Consider using a `.env` file to set up default environment variables."
)
|
|
||
|
|
||
| def prepare(self): | ||
| self.set_header("Access-Control-Allow-Origin", "*") |
There was a problem hiding this comment.
Shouldn't we add a couple of more headers? Something like:
self.set_header("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS")
self.set_header("Access-Control-Allow-Headers", "Content-Type, Authorization")
There was a problem hiding this comment.
We dont handle Authorization and tornado dont seem to mind that these headers are not part of it.
| # Patch the prepare method to allow CORS request | ||
| if os.environ.get(AQUA_CORS_ENABLE, "0") == "1": | ||
| for _, handler in __handlers__: | ||
| handler.prepare = prepare |
There was a problem hiding this comment.
Here we are dynamically patching the prepare method of handlers to set the Access-Control-Allow-Origin header based on the AQUA_CORS_ENABLE environment variable. I'm wondering, wouldn't it be better instead of patching the existing handlers, create some another base handler that will contain all the required settings, and then we can do something like this:
handlers = [
(r"/aqua/" + url, type(f"{handler.__name__}WithCorse", (handler, CORSEMixin), {}))
for url, handler in __handlers__
]
Or maybe even simpler
def patch_handler_for_cors(handler_cls) -> None:
original_prepare = getattr(handler_cls, "prepare", None)
def new_prepare(self, *args, **kwargs):
if original_prepare:
original_prepare(self, *args, **kwargs)
self.set_header("Access-Control-Allow-Origin", "*")
handler_cls.prepare = new_prepare
I don’t have a strong opinion on this, but I’m concerned that we might unintentionally override existing logic in the prepare method for one of the AQUA handlers.
There was a problem hiding this comment.
I am making the default prepare as just pass. This method is there only because the APIHandler class in the jupyterlab has this method which checks for CORS. We dont want jupyterlab's logic to come when we are launching this as a standalone. Hence I am overriding it.
Which one do you think is more readable though?
There was a problem hiding this comment.
Yours is definitely more readable. The decorator only makes sense to me if we're concerned about preserving the original prepare method that we're overriding here.
pyproject.toml
Outdated
| @@ -207,6 +207,14 @@ pii = [ | |||
| ] | |||
| llm = ["langchain>=0.2", "langchain-community", "langchain_openai", "pydantic>=2,<3", "evaluate>=0.4.0"] | |||
| aqua = ["jupyter_server"] | |||
| aquaapi = [ | |||
There was a problem hiding this comment.
Why can't we integrate this directly into [aqua]? Is jupyter_server our concern?
There was a problem hiding this comment.
We can discuss this and decide. Wanted to keep only the dependency required for the API server. I am not sure if we are using [aqua] profile anywhere other than the dev setup. If we are not using aqua anywhere else, we can remove jupyter_server and rename aquaapi to aqua
There was a problem hiding this comment.
Yeah, don't know how the aqua currently used, i thought we use it container, but we don't. I guess we can remove it, and just mention in dev docs that jupyter_server is required for dev purpose.
|
|
No description provided.