Skip to content

refactor(examples): update CSVSampleStores with old keys#611

Draft
AlessandroPomponio wants to merge 3 commits intomainfrom
ap_610_update_csv_sample_stores
Draft

refactor(examples): update CSVSampleStores with old keys#611
AlessandroPomponio wants to merge 3 commits intomainfrom
ap_610_update_csv_sample_stores

Conversation

@AlessandroPomponio
Copy link
Member

No description provided.

Signed-off-by: Alessandro Pomponio <alessandro.pomponio1@ibm.com>
@AlessandroPomponio
Copy link
Member Author

I created the following script and loaded and dumped the CSVs:

from pathlib import Path

import yaml

from orchestrator.core.samplestore.config import SampleStoreConfiguration
from orchestrator.utilities.output import pydantic_model_as_yaml

to_load = Path("examples/pfas-generative-models/gen_models_molgx_test_sample_store.yaml")
sample_store = SampleStoreConfiguration.model_validate(
    yaml.safe_load(to_load.read_text())
)
to_load.write_text(pydantic_model_as_yaml(sample_store, exclude_unset=True, exclude_none=True))

@AlessandroPomponio
Copy link
Member Author

We still need to update website/docs/actuators/replay.md:

The `propertyMap` field allows you to handle column headers had names that are
not suitable for names of properties. For example if there was a column with
measurements on a molecule called `Real_pKa (-0.83, 10.58)`, you might want to
associate this with a property called `pka` instead:
```yaml
propertyMap:
pka: "Real_pKa (-0.83, 10.58)"
```

Signed-off-by: Alessandro Pomponio <alessandro.pomponio1@ibm.com>
@AlessandroPomponio
Copy link
Member Author

@michael-johnston could you suggest a fix for the above? I think the new field is observedPropertyMap but I could be wrong

@michael-johnston
Copy link
Member

Yes should be observedPropertyMap

Copy link
Member

@michael-johnston michael-johnston left a comment

Choose a reason for hiding this comment

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

Is anything using sample_store_resource.json‎?

Seems strange it could be wrong and used?

I see this shows a problem with how we test ml_multi_cloud.
At one stage we ran tests using the files in the example folder - however this meant that we need to use absolute path for the CSV file in the example, which mean the user had to change it to run the example.

To fix this we create a separate version of the ml_multicloud_sample_store.yaml for use in unit-tests -however also update the end-to-end test in tox to use this file (as the end to end text was running from route of repo). This meant the actual example was no longer tested at all.

I think we need to also change the tox test to use the actual example file - either by running in the ml_multi_cloud example directory or by adding an override of the CSV path.

@AlessandroPomponio
Copy link
Member Author

Is anything using sample_store_resource.json‎?

Looks like the random_sql_sample_store fixture was using it

Seems strange it could be wrong and used?

It's not "wrong" because it's still an allowed syntax, we should check if there were some warnings about it though

I see this shows a problem with how we test ml_multi_cloud. At one stage we ran tests using the files in the example folder - however this meant that we need to use absolute path for the CSV file in the example, which mean the user had to change it to run the example.

To fix this we create a separate version of the ml_multicloud_sample_store.yaml for use in unit-tests -however also update the end-to-end test in tox to use this file (as the end to end text was running from route of repo). This meant the actual example was no longer tested at all.

I think we need to also change the tox test to use the actual example file - either by running in the ml_multi_cloud example directory or by adding an override of the CSV path.

I don't think this is the case - we have the ml_multi_cloud_sample_store fixture which loads the actual example. I need to look at why we have this different fixture and if we really need it

Signed-off-by: Alessandro Pomponio <alessandro.pomponio1@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

task: update all CSV sample stores referencing deprecated fields

2 participants