✨⚗️: Add service_meta_data column to identify legacy (non dy-sidecar) dynamic services #7542
Conversation
mrnicegyu11
added
a:director
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7542 +/- ##
==========================================
- Coverage 87.47% 86.56% -0.91%
==========================================
Files 1742 1327 -415
Lines 67432 54372 -13060
Branches 1144 380 -764
==========================================
- Hits 58987 47069 -11918
+ Misses 8124 7185 -939
+ Partials 321 118 -203
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
mrnicegyu11
requested review from
GitHK,
giancarloromeo,
matusdrobuliak66,
pcrespov and
sanderegg
as code owners
|
There was a problem hiding this comment.
thx! I like the idea and you triggered some interesting use cases that will allow me to extend the catalog but I left some caution notes.
At first sight, these changes will have no effect on the service_meta_data table of current deploys (because of point 1 in my comment) but I can modify the background task to identify editable and non-editable.
Nonetheless, we should double check
- added columns in
ServiceMetaDataPublished - heuristics in the director that determines that this service is "legacy" (BTW have you checked how is done in the directorv2/sidecars ?)
| await services_repo.create_or_update_service( | ||
| ServiceMetaDataDBCreate( | ||
| **service_metadata.model_dump(exclude_unset=True), owner=owner_gid | ||
| **service_metadata.model_dump(exclude_unset=True, exclude={""}), |
| if not key.startswith("io.simcore."): | ||
| if not key.startswith("io.simcore.") and not key.startswith( | ||
| "simcore.service." | ||
| ): # Keeping "simcore.service." adds additonally input_paths, output_paths, state_paths |
There was a problem hiding this comment.
I would add some tests on these conditions.
how can we guarantee these heuristics are not discarding other services or future services?
is there any other way e.g. via intergration-version to more reliably identify "legacy dynamic-service"?
| label_data = json.loads(labels[key]) | ||
| for label_key in label_data: | ||
| if isinstance( | ||
| label_key, dict |
There was a problem hiding this comment.
- AFAIK a dict can only use has hashable types as keys, i.e.
>>> d = {}
>>> d[d] = 33
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'dict'
so the only case i can imagine is that label_data is a list[dict| ... ] or some equivalent iterable that might return dicts.
Is this condition ONLY applicable to "simcore.service." ?
There was a problem hiding this comment.
label_data is in fact sometimes a list of dicts ;)
| alias="progress_regexp", | ||
| description="regexp pattern for detecting computational service's progress", | ||
| ) | ||
| inputs_path: str | None = Field( |
| op.add_column( | ||
| "services_meta_data", | ||
| sa.Column( | ||
| "is_classic_dynamic_service", |
There was a problem hiding this comment.
The purpose of copying certain labels from the image registry into columns in the service_meta_data table is threefold:
- To retain a local copy of available registry services (e.g. identifiers).
- To allow certain fields to be editable (e.g., service owners can modify the original description. SEE also the columns doc-ed as
(editable)). - To serve as a searchable cache, avoiding direct dependency on the registry service.
A flag like is_classic_dynamic_service is non-editable, as it reflects an immutable property (unlike, say, a description). While I'm not entirely fond of including it, I acknowledge its usefulness.
But now we face a couple of challenges to solve:
- Update of Services: The background task does not overwrite existing rows for services already present in the table. This is intentional, to preserve user-editable fields (e.g., if a user updates a service description, it shouldn't be reverted). As a result, flags like
is_classic_dynamic_servicewill only be updated for new services or from an empty table. - Single Source of Truth: Other simcore-services (e.g.,
director-v2,sidecar, etc.) must consistently rely on this flag as the single source of truth, rather than applying their own heuristics or deriving the same information independently.
| ) | ||
| inputs_path: str | None = Field( | ||
| None, | ||
| description="if this is present, the service is a modern style dv2 dynamic service", |
There was a problem hiding this comment.
If so, I am surprised that was not already here ...
CAREFUL with changes in this class since it is coupled in many projects. Specifically with the .osparc/metadata.yml file used in OOIL
There was a problem hiding this comment.
I am generally against such a thing for the following reasons:
- the services metadata table entries are made to be editable, this is now something different
- this will need to be synchronised, I see in another PR and also after our discussions that we are upgrading these services. How will this be handled?
I think there is already some code in the dynamic sidecar and director-v2 in order to find out whether a service is legacy or not. It is preferable to have a function for this as this is "dynamic" if you allow the term and not something cached in the database.
| alias="progress_regexp", | ||
| description="regexp pattern for detecting computational service's progress", | ||
| ) | ||
| strip_path: str | None = Field( |
There was a problem hiding this comment.
I also tend not to agree with these changes. Your source of truth is the simcore.service.paths-mapping label, if it's not None it's a new style dynamic service.
I would rather have an API endpoint on the catalog service instead of adding something to the DB. I also agree with PC's statement regarding the reasons of having the data present in the db.
|
alright thanks for your comments, I will close this PR then |
5 participants
What do these changes do?
For me, it is always confusing to not be able to identify classic (sometimes called
legacy, I mean before-dy-sidecar) services from the postgres DB.FYI: I don't have my heart set on merging this PR. I can run it locally from my fork and can point dv0 to the
registry.osparc.ioendpoint to compile the necessary information on my local machine in the local postgres. I am presenting this here simply for your consideration and as an informer. Maybe someone finds it useful to have this information in sight.Related issue/s
ITISFoundation/osparc-issues#1866
How to test
Dev-ops checklist