-
Notifications
You must be signed in to change notification settings - Fork 27
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
update requirements #222
update requirements #222
Conversation
# https://docs.pydantic.dev/latest/api/config/#pydantic.alias_generators.to_camel | ||
"alias_generator": camelize, | ||
"populate_by_name": True, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
# https://github.com/pydantic/pydantic/discussions/6388 | ||
# class Config: | ||
# json_loads = orjson.loads | ||
# json_dumps = orjson_dumps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't customize this with pydantic 2 but it seems that pydantic 2 is faster anyway
|
||
base_conformance_classes.extend(self.extra_conformance_classes) | ||
|
||
return sorted(list(set(base_conformance_classes))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -34,6 +33,4 @@ async def _fetch() -> Dict: | |||
return queryables | |||
|
|||
cache_key = f"{CACHE_KEY_QUERYABLES}:{collection_id}" | |||
queryables = await cached_result(_fetch, cache_key, request) | |||
headers = {"Content-Type": "application/schema+json"} | |||
return JSONResponse(queryables, headers=headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
await connect_to_db(app) | ||
await connect_to_redis(app) | ||
yield | ||
await close_db_connection(app) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startup/shutdown events are deprecated and lifespan
is the new
way to do
search_get_request_model=search_get_request_model, | ||
search_post_request_model=search_post_request_model, | ||
response_class=ORJSONResponse, | ||
exceptions={**DEFAULT_STATUS_CODES, **PC_DEFAULT_STATUS_CODES}, | ||
middleware=[ | ||
Middleware(BrotliMiddleware), | ||
Middleware(ProxyHeaderMiddleware), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both are defined in the default stac-fastapi so they should have been present in the application before
I don't seem to be able to run the tests locally:
I cannot replicate outside the docker env, this might be an issue when compiling pydantic rust code 🤷 |
# Mypi is complaining about this with | ||
# error: Incompatible types (expression has type "str", | ||
# TypedDict item "extra" has type "Extra") | ||
"extra": "ignore", # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: remove type: ignore
and see if this pass in the CI
@@ -74,12 +74,12 @@ def test_get_render_config() -> None: | |||
|
|||
def test_render_config_parse_max_items() -> None: | |||
config = { | |||
"render_params": [], | |||
"render_params": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how this passed before
@@ -1,4 +1,4 @@ | |||
FROM mcr.microsoft.com/azure-functions/python:4-python3.8 | |||
FROM mcr.microsoft.com/azure-functions/python:4-python3.10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can revert to 3.8 if needed
env_nested_delimiter = "__" | ||
model_config = { | ||
"env_prefix": ANIMATION_SETTINGS_PREFIX, | ||
"env_nested_delimiter": "__", # type: ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue locally with mypy complaining about error: Incompatible types (expression has type "str",
settings = ImageSettings() # type: ignore | ||
logger.info(f"API URL: {settings.api_root_url}") | ||
logger.info(f"Concurrency limit: {settings.tile_request_concurrency}") | ||
return settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced ImageSettings.get
with an external function which should gives the same result.
FYI: Pydantic was complaining about the .get
method
|
||
from pcstac.config import get_settings | ||
|
||
# Use a TypeAdapter to parse any datetime strings in a consistent manner | ||
UtcDatetimeAdapter = TypeAdapter(UtcDatetime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stac-pydantic now use pydantic Datetime parsing to validate datetime string
@mmcfarland I'll all done with the dependency updates! Sadly the I'm not quite aware of the interaction pattern between the Client and the API so I'm just going to 🤞 😅 |
Great! I'll pull this down and do some testing and let you know how things are looking! |
FYI: there are some breaking changes that needs to happen in stac-fastapi (ref: stac-utils/stac-fastapi#713) so we might need to update this PR |
@@ -50,6 +55,15 @@ | |||
|
|||
app_settings = get_settings() | |||
|
|||
items_get_request_model = PCItemCollectionUri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can now directly define the GET - /items
model. This wasn't used previously
Superseded by #233 -- thanks for this baseline @vincentsarago! |
Description
This is the first PR which will update the requirements for the different PC services.
The test should fail right now, but I wanted to see if they were passing for
pccommon
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
Please delete options that are not relevant.