Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #236 +/- ##
===========================================
+ Coverage 87.41% 87.47% +0.05%
===========================================
Files 205 206 +1
Lines 12461 12568 +107
Branches 1049 1065 +16
===========================================
+ Hits 10893 10994 +101
- Misses 1181 1185 +4
- Partials 387 389 +2 ☔ View full report in Codecov by Sentry. |
| return firebase_models.FbMappingTaskStreetCreateOnlyInput( | ||
| # XXX: converting this to int for backwards compatibility | ||
| taskId=int(task.firebase_id), | ||
| taskId=task.firebase_id, |
There was a problem hiding this comment.
Is there any reason to remove the casting of firebase_id?
There was a problem hiding this comment.
Yes, we use the street-level image ID as the MapSwipe task ID so that we can easily link MapSwipe results back to the original images without having to add an additional task attribute.
However, whereas Mapillary has integer image identifiers, Panoramax uses UUID v4 for identifiers.
There was a problem hiding this comment.
We should still cast it to int for backward compatibility if the provider is Mapillary
There was a problem hiding this comment.
Shouldn't this be in street/project.py?
There was a problem hiding this comment.
This is consistent with raster_tile_server and vector_tile_server also in utils/geo/
|
|
||
| # Type hints | ||
| id: int | ||
| id: int | str |
There was a problem hiding this comment.
Any reason on changing the type?
There was a problem hiding this comment.
Same as above: task id == Panoramax image ID (UUID v4)
There was a problem hiding this comment.
This is just a type hint.
How was "id" change to "str" in the database.
Also, why change the internal "id" representation?
There was a problem hiding this comment.
I see. This was a mix-up with the firebase ID, the internal id representation does not need to be changed indeed.
71352ae to
f15f817
Compare
a2dad0a to
f15f817
Compare
|
|
||
| # Type hints | ||
| id: int | ||
| id: int | str |
There was a problem hiding this comment.
This is just a type hint.
How was "id" change to "str" in the database.
Also, why change the internal "id" representation?
| else: | ||
| raise Exception(f"Unknown provider {getattr(provider.name, 'value', '')}") |
There was a problem hiding this comment.
Use assert_never so that we get static type checks.
| endTime: String | ||
| isPano: Boolean | ||
| organizationId: String | ||
| panoOnly: Boolean |
There was a problem hiding this comment.
This might require a migration.
There was a problem hiding this comment.
To my understanding:
- A migration is not required as the attribute is stored in JSON fields, not in a dedicated column
- We probably do not need a backfill neither, as this would only be a potential issue for unpublished project drafts created with the former version having isPano: true. The filter is only used during project processing.
There was a problem hiding this comment.
The data in the system should conform with the schema.
There can be 2 different scenarios that will be problematic if we do not migrate/backfill the data:
- The data does not confirm with the schema at all
- This will raise validation issue on graphql (serialization).
- This will directly impact UI for all the existing projects (draft, completed, etc.)
- There will be no validation issue but the system will interpret the data differently (e.g. non-mandatory fields renamed)
| """Numeric value as string""" | ||
| aoiGeometry: String! | ||
| customOptions: [CustomOptionInput!] = null | ||
| imageProvider: StreetImageProviderInput = null |
There was a problem hiding this comment.
This might require a migration.
There was a problem hiding this comment.
This is a nested key inside the JSON field project_type_specifics and does not create a new column.
Backend treats it as optional and defaults to MAPILLARY, so existing records without imageProvider continue to validate.
There was a problem hiding this comment.
We should not have fallback pattern on the client-side. That will cause 2 source of truth.
Instead this field should be mandatory and the data should be migrated/back-filled.
| """Numeric value as string""" | ||
| aoiGeometry: String! | ||
| customOptions: [ProjectCustomOption!] | ||
| imageProvider: StreetImageProvider |
There was a problem hiding this comment.
This might require a migration.
| return firebase_models.FbMappingTaskStreetCreateOnlyInput( | ||
| # XXX: converting this to int for backwards compatibility | ||
| taskId=int(task.firebase_id), | ||
| taskId=task.firebase_id, |
There was a problem hiding this comment.
We should still cast it to int for backward compatibility if the provider is Mapillary
| elif provider.name in (StreetImageProviderNameEnum.PANORAMAX, StreetImageProviderNameEnum.PANORAMAX_CUSTOM): | ||
| base = (provider.url or Config.PANORAMAX_API_LINK).rstrip("/") | ||
| url = f"{base}/api/map/{z}/{x}/{y}.mvt" |
There was a problem hiding this comment.
What is the different between PANORMAX and PANORAMAX_CUSTOM?
Do we need both of these?
Also, the code above indicates that user can override MAPILLARY url
There was a problem hiding this comment.
The difference is:
- PANORAMAX uses the default public Metacatalog API endpoint with access to images of all federated Panoramax instances.
- PANORAMAX_CUSTOM allows users to specify a custom Panoramax API URL and thereby uses images from unfederated instances, or filter for images from a specific Panoramax instance.
Both use the same API format (/api/map/{z}/{x}/{y}.mvt) and return the same data structure, but PANORAMAX_CUSTOM is validated to require a URL.
The requirement to add an explicit PANORAMAX_CUSTOM came from review of the changes in Manager Dashboard: mapswipe/manager-dashboard#74 (review)
I agree that the user should not be able to override the MAPILLARY url, as the Pydantic validator in StreetImageProvider already enforces. I pushed a commit that clearly indicates that we always call the Mapillary API at Config.MAPILLARY_API_LINK.
Add logic from mapswipe/python-mapswipe-workers@dev...panoramax and refactor StreetImageProvider. This introduces a type change of View Streets project tasks taskId that requires migration and changes in Firebase.
…nsure correct sorting, sampling threshhold input in m instead km
Co-authored-by: Sushil Tiwari <susiltiwari750@gmail.com>
…ider is mapillary, correct type hint
… mandatory image provider
…tespace in migration
d4cb5bf to
5a5553c
Compare
Depends on
Changes
This enables the MapSwipe backend to handle View Streets projects with different image providers (currently Mapillary and Panoramax).
This PR doesn't introduce any:
printThis PR contains valid: