Feature/update computational asset definition

[hbonell-iti]

Add some properties and models from computational asset definition:

Models:

• Accelerator
• CPU
• Memory
• Storage

Properties:

• os
• kernel
• pricing_scheme
• cpu
• memory
• accelerator
• storage

[PGijsbers]

Sorry for the delay in reviewing. It's been very hectic. I will review this at the latest next week.

[PGijsbers]

I noticed no API endpoints have been added. Is this deliberate?

[hbonell-iti]

Yes, it is deliberated, we just change the computational asset model adding some needed properties.

[PGijsbers]

Overall, I think it looks fine. For documentation purposes/users, it would be good to document examples and the units used for each field.

[PGijsbers]

Would it make sense for the components (accelerator, cpu, etc.) to be many-to-many? E.g., one CPU model may be used in different computational assets. Perhaps this is not common enough to avoid duplication in the database?

[PGijsbers]

@hbonell-iti @soniasantiago It's marked ready to (re)review but I would like to still know if what's the thought process here. It doesn't look like Cpu is intended to specify one particular unit, but rather a model. In which case I assume it can occur in different computational assets?

[soniasantiago]

Apologies for not answering to your comment before.
As you say, it's true that one CPU model could be used in different computational assets. However, due to the different attributes of the CPU model and its possible values, we believe it won't happen too often. This is the main reason why the relation is defined one-to-many.

However, if you think this is not the best approach, let us know and we will do in the other way.

[PGijsbers]

I am fine going with your judgement, I just wanted to ensure it wasn't an oversight.

[soniasantiago]

Perfect. I just wanted to make sure that this is done in the right way. Thanks for checking this!

[soniasantiago]

@PGijsbers Besides the open comments, since the computational asset model has changed, I'm not sure if we should implement the corresponding database migration script. I have checked that in the production instance there is only one example computational asset defined, so dropping the computational_asset table and recreating it might be enough. But, if not, let us know and we will do that.

For the "location" relationship, when we deployed in our dev environment this version of the API REST we had to apply in the database the following modification:

SET FOREIGN_KEY_CHECKS = 0;

ALTER TABLE location ADD COLUMN computational_asset_identifier INT NULL;

CREATE INDEX ix_location_computational_asset_identifier ON location(computational_asset_identifier);

ALTER TABLE location 
ADD CONSTRAINT fk_location_computational_asset
FOREIGN KEY (computational_asset_identifier)
REFERENCES computational_asset(identifier)
ON DELETE CASCADE;
 
SET FOREIGN_KEY_CHECKS = 1;

so we may have to define this in a database migration script.

[PGijsbers]

Sorry for not catching this the first time around, but changes to existing tables also need database schema migration scripts. I linked the docs in another comment. Let me know if you need more help.

I would recommend taking the following steps:

• Merge develop into this branch, or rebase (this is to ensure you have all current migration scripts).
• Make sure you can upload a computational asset through your local rest API (preferably also automate this in a unit test). This verifies the current schema works as intended, so you can build a migration script off that.¹
• Then go to the develop branch to recreate the dataset with the current schema:

docker compose --profile "*" down
git checkout develop
docker compose up -d

• Then go back to this branch and run the migration script you created.
• If the migration script is correct, you should still be able to upload a computational asset. If it is incorrect, you will likely get an error telling what column(s) are missing. In that case, update the script and repeat the last two steps.

Here's an example migration script adding the "os" column. I would recommend to extend it to contain all changes of this PR into the same Alembic migration:

"""add os to computationalasset

Revision ID: 35d4190fa3c6
Revises: 0f2fe1324047
Create Date: 2024-11-20 14:15:48.456218

"""
from typing import Sequence, Union

from alembic import op
import sqlalchemy as sa
from sqlalchemy import String, Column

from database.model.field_length import NORMAL

# revision identifiers, used by Alembic.
revision: str = '35d4190fa3c6'
down_revision: Union[str, None] = '0f2fe1324047'
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None


def upgrade() -> None:
  op.add_column(
      f"computational_asset",
      Column("os", String(NORMAL)),
  )


def downgrade() -> None:
  pass

To recreate the database at any point (with loss of data):

docker compose down
./scripts/clean.sh
docker compose up -d

Footnotes

1. I haven't yet experimented with the automatic generation of migration scripts, but it might be worth looking into. ↩

[PGijsbers]

For changes to tables which already exist in the database, we need to add a database schema migration script. Please see this documentation for more information.

[soniasantiago]

Perfect, thanks for the reference!

[PGijsbers]

As I read the metadata model, CPU should be a subclass of AIoD concept?

[PGijsbers]

Same for accelerator, and other added items?

[soniasantiago]

We thought about these entities to be similarly as "location". That is, not a subclass of AIoD concept, at least at the implementation level.

[PGijsbers]

Then maybe I misunderstand how to read
this documentation. I am having a meeting with Antonis about this next week.

[hbonell-iti]

The migration file was added. Is there anything else to add to the pull request?