[DRCC][AQUA] Completely Removing OSS bucket config dependency #1147

kumar-shivam-ranjan · 2025-04-04T19:00:38Z

Description

This PR is intended to completely remove the dependency of AQUA on config stored in Object storage bucket. Also , this change would ensure Aqua will be available for all realms even when no ODSC_MODEL_COMPARTMENT_OCID env variable is set.

Unit Tests

mrDzurb · 2025-04-07T02:21:56Z

ads/aqua/__init__.py

@@ -41,5 +40,5 @@ def set_log_level(log_level: str):
    set_auth("resource_principal")

 ODSC_MODEL_COMPARTMENT_OCID = (
-    os.environ.get("ODSC_MODEL_COMPARTMENT_OCID") or fetch_service_compartment()
+    os.environ.get("ODSC_MODEL_COMPARTMENT_OCID") or "ODSC_MODEL_COMPARTMENT_OCID"


I'm not sure that I understand this logic. In one case we return the value of the ODSC_MODEL_COMPARTMENT_OCID env var, but if it is not defined we just return the name of the env var?

Currently , service model compartment ocid is used as a key to cache service model list response. Now since we won't be fetching this compartment id in ADS from container_index.json , just replaced it with constant string.
If we explicitly set this compartment ocid via NB environment variables , then corresponding ocid will be used as key.

Should we just remove ODSC_MODEL_COMPARTMENT_OCID?
I need to check if we can still use non prod backend for fetching service models list incase we do that since this is only place i feel this env variable is being used.

Here i'm not sure. My vote would be for removing this variable, but @VipulMascarenhas might have a different opinion. We might use this env variable to identify if the model is service or a custom one.

Tested the flow after removing ODSC_MODEL_COMPARTMENT_OCID. Seems like we don't need this env variable for fetching service models even for non prod backend. The new List models API would take care of it.
We can rename the key for storing service models list response in cache.

So , everything seems fine except there is only once place this is being used where i am not sure it is safe to remove.
CC: @VipulMascarenhas

if service_model.compartment_id != ODSC_MODEL_COMPARTMENT_OCID: logger.info( f"Aqua Model {model_id} already exists in the user's compartment." "Skipped copying." ) return service_model

Is it safe to remove this condition?

Replaced it with

# Skip model copying if it is registered model if service_model.freeform_tags.get(Tags.BASE_MODEL_CUSTOM) is not None: logger.info( f"Aqua Model {model_id} already exists in the user's compartment." "Skipped copying." ) return service_model

mrDzurb · 2025-04-07T02:25:34Z

ads/aqua/extension/utils.py

@@ -41,14 +40,14 @@ def ui_compatability_check():
    """This method caches the service compartment OCID details that is set by either the environment variable or if
    fetched from the configuration. The cached result is returned when multiple calls are made in quick succession
    from the UI to avoid multiple config file loads."""
-    return ODSC_MODEL_COMPARTMENT_OCID or fetch_service_compartment()
+    return ODSC_MODEL_COMPARTMENT_OCID


Do we really need this compatibility check method now?

Yes , can be removed.
Will remove

Updated. Thanks

…endency

VipulMascarenhas · 2025-04-07T21:08:28Z

ads/aqua/extension/common_ws_msg_handler.py

@@ -39,19 +36,8 @@ def process(self) -> Union[AdsVersionResponse, CompatibilityCheckResponse]:
            )
            return response
        if request.get("kind") == "CompatibilityCheck":
-            if ui_compatability_check():


if this always returns ok, does UI need to call this? This won't be needed, right?

This is compatibility check handler implemented in websockets. We call the one implemented via HTTP. Both the methods should return ok always as we decide to enable AQUA everywhere.

Yes , we can remove calling this API in UI. I kept it since may be in future we can add some sort of AQUA enabling logic based on CP config. Are you suggesting we removing this api alltogether from ADS?

Updated. Thanks

VipulMascarenhas · 2025-04-07T21:14:01Z

ads/aqua/model/model.py

-        if service_model.compartment_id != ODSC_MODEL_COMPARTMENT_OCID:
+
+        # Skip model copying if it is registered model
+        if service_model.freeform_tags.get(Tags.BASE_MODEL_CUSTOM) is not None:


can freeform_tags be None? I don't think it's going happen for aqua models, but can we add a check here to return appropriate error instead if for some reason these tags are empty.

Will update.

Updated. Thanks

VipulMascarenhas · 2025-04-07T21:37:38Z

ads/aqua/model/model.py

                logger.info(
-                    f"Returning service models list in {ODSC_MODEL_COMPARTMENT_OCID} from cache."
+                    f"Returning service models list in {AQUA_SERVICE_MODELS} from cache."


nit: might be good to just log this as Returning service models list from cache.

Updated. Thanks

VipulMascarenhas · 2025-04-07T21:39:29Z

ads/aqua/model/model.py

            logger.info("Fetching service models.")
            lifecycle_state = kwargs.pop(
                "lifecycle_state", Model.LIFECYCLE_STATE_ACTIVE
            )

            models = self.list_resource(
                self.ds_client.list_models,
-                compartment_id=ODSC_MODEL_COMPARTMENT_OCID,
+                compartment_id=compartment_id or COMPARTMENT_OCID,


wondering if we need to filter the model list here based on service model tags. If we list from users compartment, this would only return service models and not other registered models from users compartment?

Not sure if i fully understand your question
but this would return either service models or user models based on category. If category is user (default) , it would return all registered models from user's compartment

Updated. Thanks

VipulMascarenhas · 2025-04-07T21:43:51Z

ads/aqua/model/model.py

                **kwargs,
            )

        logger.info(
-            f"Fetched {len(models)} model in compartment_id={ODSC_MODEL_COMPARTMENT_OCID if category==SERVICE else compartment_id}."
+            f"Fetched {len(models)} models from {AQUA_SERVICE_MODELS if category==SERVICE else compartment_id}."


the message might not be clear with the new service model key, let's just log:
Fetched x service models.
or
Fetched x models from compartment <compartment_id>

Sure , will update

Updated. Thanks

github-actions · 2025-04-07T22:35:49Z

📌 Cov diff with main:

📌 Overall coverage:

….com/oracle/accelerated-data-science into feature/oss-bucket-config-dependency

github-actions · 2025-04-08T10:43:56Z

📌 Cov diff with main:

📌 Overall coverage:

github-actions · 2025-04-08T23:35:40Z

📌 Cov diff with main:

📌 Overall coverage:

kumar-shivam-ranjan added 2 commits April 5, 2025 00:04

Removing oss bucket config

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

f62eb5d

Enhancing list model api + config dependency removal

Loading
Loading status checks…

7795394

kumar-shivam-ranjan requested review from darenr, mayoor, mrDzurb, VipulMascarenhas, qiuosier and ahosler as code owners April 4, 2025 19:00

oracle-contributor-agreement bot added the OCA Verified label Apr 4, 2025

Removing print statements

Loading
Loading status checks…

20933af

mrDzurb reviewed Apr 7, 2025

View reviewed changes

Removing ODSC_MODEL_COMPARTMENT_OCID

Loading
Loading status checks…

eb88b11

kumar-shivam-ranjan requested a review from mrDzurb April 7, 2025 04:52

Merge branch 'aqua/ADS_MS_changes' into feature/oss-bucket-config-dep…

9c5d54f

…endency

kumar-shivam-ranjan added the AQUA label Apr 7, 2025

kumar-shivam-ranjan self-assigned this Apr 7, 2025

VipulMascarenhas reviewed Apr 7, 2025

View reviewed changes

kumar-shivam-ranjan changed the base branch from aqua/ADS_MS_changes to main April 7, 2025 22:04

Merge branch 'main' into feature/oss-bucket-config-dependency

Loading
Loading status checks…

f10ac55

kumar-shivam-ranjan added 3 commits April 8, 2025 13:11

Merge branch 'feature/oss-bucket-config-dependency' of https://github…

d7affcc

….com/oracle/accelerated-data-science into feature/oss-bucket-config-dependency

Fixing env variable override issue

3ed30fa

Removing from_container_index method

Loading
Loading status checks…

a80395a

kumar-shivam-ranjan added 2 commits April 9, 2025 04:29

Addressing review comments

Loading
Loading status checks…

e5ee1bb

Updating logging messages

Loading
Loading status checks…

644d53c

mrDzurb approved these changes Apr 9, 2025

View reviewed changes

VipulMascarenhas approved these changes Apr 9, 2025

View reviewed changes

kumar-shivam-ranjan merged commit 789786f into main Apr 9, 2025
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRCC][AQUA] Completely Removing OSS bucket config dependency #1147

[DRCC][AQUA] Completely Removing OSS bucket config dependency #1147

kumar-shivam-ranjan commented Apr 4, 2025

mrDzurb Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

mrDzurb Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

mrDzurb Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

kumar-shivam-ranjan Apr 8, 2025

VipulMascarenhas Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025 •

edited

Loading

kumar-shivam-ranjan Apr 7, 2025 •

edited

Loading

kumar-shivam-ranjan Apr 8, 2025

VipulMascarenhas Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

kumar-shivam-ranjan Apr 8, 2025

VipulMascarenhas Apr 7, 2025

kumar-shivam-ranjan Apr 8, 2025

VipulMascarenhas Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

kumar-shivam-ranjan Apr 8, 2025

VipulMascarenhas Apr 7, 2025

kumar-shivam-ranjan Apr 7, 2025

kumar-shivam-ranjan Apr 8, 2025

github-actions bot commented Apr 7, 2025

github-actions bot commented Apr 8, 2025

github-actions bot commented Apr 8, 2025

[DRCC][AQUA] Completely Removing OSS bucket config dependency #1147

[DRCC][AQUA] Completely Removing OSS bucket config dependency #1147

Conversation

kumar-shivam-ranjan commented Apr 4, 2025

Description

Unit Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar-shivam-ranjan Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

kumar-shivam-ranjan Apr 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 7, 2025

github-actions bot commented Apr 8, 2025

github-actions bot commented Apr 8, 2025

kumar-shivam-ranjan Apr 7, 2025 •

edited

Loading

kumar-shivam-ranjan Apr 7, 2025 •

edited

Loading