Skip to content

✨⚗️: Add service_meta_data column to identify legacy (non dy-sidecar) dynamic services #7542

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.

Sign up for GitHub

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

Closed
wants to merge 10 commits into from
Closed

✨⚗️: Add service_meta_data column to identify legacy (non dy-sidecar) dynamic services #7542

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
93d810a
fix typo
mrnicegyu11 Apr 15, 2025
fe8526b
init
mrnicegyu11 Apr 15, 2025
0dcca0c
Bugfix
mrnicegyu11 Apr 15, 2025
7f26d3b
Adjust comment
mrnicegyu11 Apr 15, 2025
31c5082
Bugfixes
mrnicegyu11 Apr 16, 2025
f3896b1
Rename legacy --> classic
mrnicegyu11 Apr 16, 2025
5b19042
Merge branch 'master' into 2025/addLegacyServiceIdentifierToDB
mrnicegyu11 Apr 16, 2025
50a72a0
Fix remove comment
mrnicegyu11 Apr 16, 2025
30e2cd9
Bugfixes
mrnicegyu11 Apr 16, 2025
97b267a
Bugfixes
mrnicegyu11 Apr 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/models-library/src/models_library/services_metadata_published.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,18 @@ class ServiceMetaDataPublished(ServiceKeyVersion, ServiceBaseDisplay):
alias="progress_regexp",
description="regexp pattern for detecting computational service's progress",
)
inputs_path: str | None = Field(
Copy link
Member

@pcrespov pcrespov Apr 16, 2025

Choose a reason for hiding this comment

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

is this a Path?

None,
description="if this is present, the service is a modern style dv2 dynamic service",
Copy link
Member

@pcrespov pcrespov Apr 16, 2025

Choose a reason for hiding this comment

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

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

https://github.com/itisfoundation/osparc-simcore/blob/c37d45b4ad9987eb30c6c3b1958f7d3e908354d5/packages/service-integration/src/service_integration/osparc_config.py#L99-L104

)
outputs_path: str | None = Field(
None,
description="if this is present, the service is a modern style dv2 dynamic service",
)
state_paths: str | None = Field(
None,
description="if this is present, the service is a modern style dv2 dynamic service",
)

# SEE https://github.com/opencontainers/image-spec/blob/main/annotations.md#pre-defined-annotation-keys
image_digest: str | None = Field(
Expand Down
36 changes: 36 additions & 0 deletions ...gres_database/migration/versions/354b5d921312_introduce_legacy_service_identification_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
"""Introduce legacy service identification column
Revision ID: 354b5d921312
Revises: cf8f743fd0b7
Create Date: 2025-04-15 13:53:44.404695+00:00
"""

import sqlalchemy as sa
from alembic import op

# revision identifiers, used by Alembic.
revision = "354b5d921312"
down_revision = "cf8f743fd0b7"
branch_labels = None
depends_on = None


def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.add_column(
"services_meta_data",
sa.Column(
"is_classic_dynamic_service",
Copy link
Member

@pcrespov pcrespov Apr 16, 2025

Choose a reason for hiding this comment

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

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:

  1. 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_service will only be updated for new services or from an empty table.
  2. 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.

sa.Boolean(),
server_default=sa.text("false"),
nullable=False,
),
)
# ### end Alembic commands ###


def downgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_column("services_meta_data", "is_classic_dynamic_service")
# ### end Alembic commands ###
7 changes: 7 additions & 0 deletions packages/postgres-database/src/simcore_postgres_database/models/services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,13 @@
doc="Timestamp when the service is retired (editable)."
"A fixed time before this date, service is marked as deprecated.",
),
sa.Column(
"is_classic_dynamic_service",
sa.Boolean,
nullable=False,
server_default=sa.false(),
doc="Is the service a legacy-style classic dynamic service? This is false for comp-services and special items like filepickers.",
),
sa.PrimaryKeyConstraint("key", "version", name="services_meta_data_pk"),
)

Expand Down
9 changes: 8 additions & 1 deletion services/catalog/src/simcore_service_catalog/core/background_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,16 @@ def _by_version(t: tuple[ServiceKey, ServiceVersion]) -> Version:
)

# set the service in the DB
_is_classic_dynamic_service: bool = (
service_metadata.inputs_path is None
and service_metadata.outputs_path is None
and service_metadata.state_paths is None
)
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={""}),
Copy link
Member

@pcrespov pcrespov Apr 16, 2025

Choose a reason for hiding this comment

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

what is this exclude={""} ?

owner=owner_gid,
_is_classic_dynamic_service=_is_classic_dynamic_service,
),
service_access_rights,
)
Expand Down
1 change: 1 addition & 0 deletions services/catalog/src/simcore_service_catalog/models/services_db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class ServiceMetaDataDBCreate(BaseModel):

# lifecycle
deprecated: datetime | None = None
_is_classic_dynamic_service: bool = False

@staticmethod
def _update_json_schema_extra(schema: JsonDict) -> None:
Expand Down
2 changes: 1 addition & 1 deletion services/director/src/simcore_service_director/api/rest/_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ async def list_services(
the_app, registry_proxy.ServiceType.DYNAMIC
)
# NOTE: the validation is done in the catalog. This entrypoint IS and MUST BE only used by the catalog!!
# NOTE2: the catalog will directly talk to the registry see case #2165 [https://github.com/ITISFoundation/osparc-simcore/issues/2165]
# NOTE2: the catalog might eventually directly talk to the registry see case #2165 [https://github.com/ITISFoundation/osparc-simcore/issues/2165]
# services = node_validator.validate_nodes(services)
return Envelope[list[dict[str, Any]]](data=services)
except RegistryConnectionError as err:
Expand Down
8 changes: 7 additions & 1 deletion services/director/src/simcore_service_director/registry_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,11 +410,17 @@ async def get_image_details(
if not labels:
return image_details
for key in labels:
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
Copy link
Member

@pcrespov pcrespov Apr 16, 2025

Choose a reason for hiding this comment

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

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"?

continue
try:
label_data = json.loads(labels[key])
for label_key in label_data:
if isinstance(
label_key, dict
Copy link
Member

@pcrespov pcrespov Apr 16, 2025

Choose a reason for hiding this comment

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

  • 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." ?

Copy link
Member Author

@mrnicegyu11 mrnicegyu11 Apr 16, 2025

Choose a reason for hiding this comment

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

label_data is in fact sometimes a list of dicts ;)

): # Dicts from "simcore.service." docker image labels are omitted.
continue
image_details[label_key] = label_data[label_key]
except json.decoder.JSONDecodeError:
logging.exception(
Expand Down
6 changes: 3 additions & 3 deletions services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/long_running_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ async def _send_resource_tracking_stop(platform_status: SimcorePlatformStatus):
progress.update(message="done", percent=ProgressPercent(0.99))


def _get_satate_folders_size(paths: list[Path]) -> int:
def _get_state_folders_size(paths: list[Path]) -> int:
total_size: int = 0
for path in paths:
for file in path.rglob("*"):
Expand Down Expand Up @@ -406,7 +406,7 @@ async def task_restore_state(
)
progress.update(message="state restored", percent=ProgressPercent(0.99))

return _get_satate_folders_size(state_paths)
return _get_state_folders_size(state_paths)


async def _save_state_folder(
Expand Down Expand Up @@ -471,7 +471,7 @@ async def task_save_state(
await post_sidecar_log_message(app, "Finished state saving", log_level=logging.INFO)
progress.update(message="finished state saving", percent=ProgressPercent(0.99))

return _get_satate_folders_size(state_paths)
return _get_state_folders_size(state_paths)


async def task_ports_inputs_pull(
Expand Down
Loading