Skip to content
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.

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

Feature store marketplace operator #441

Merged
merged 125 commits into from
Jan 24, 2024

Conversation

harsh97
Copy link
Member

@harsh97 harsh97 commented Nov 21, 2023

An operator to assist in setting up feature store environment in tenancy via marketplace.
Steps to test:
ads opctl configure
ads init -t feature_store_marketplace
ads run -f <--generated--yaml--> -b marketplace

For more details refer documentation:

  1. cd /ads/feature_store/docs
  2. make livehtml
  3. Go to this url for documentation http://127.0.0.1:8000/user_guides/setup/feature_store_operator.html

Screenshot from 2024-01-18 17-10-04

@harsh97 harsh97 requested a review from KshitizLohia November 21, 2023 04:11
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Nov 21, 2023
Copy link

📌 Cov diff with main:

Coverage-3%

📌 Overall coverage:

Coverage-24.06%

Copy link

📌 Cov diff with main:

Coverage-3%

📌 Overall coverage:

Coverage-24.06%

Copy link

📌 Cov diff with main:

Coverage-3%

📌 Overall coverage:

Coverage-24.05%

Copy link

📌 Cov diff with main:

No success to gather report. 😿

📌 Overall coverage:

No success to gather report. 😿

Copy link

📌 Cov diff with main:

Coverage-63%

📌 Overall coverage:

Coverage-62.23%

@@ -1,17 +1,21 @@
Transformation
**************

Transformations in a feature store refers to the operations and processes applied to raw data to create, modify or derive new features that can be used as inputs for ML Models. These transformations are crucial for improving the quality, relevance and usefulness of features which in turn can enhance the performance of ml models. It is an object that represents a transformation applied on the feature group and can be a pandas transformation or spark sql transformation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the contract user must follow while authoring these transformations

Copy link
Member

Choose a reason for hiding this comment

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

Added the contract as part of the documentation

Copy link

⚠️ This PR changed pyproject.toml file. ⚠️

  • PR Creator must update 📃 THIRD_PARTY_LICENSES.txt, if any 📚 library added/removed in pyproject.toml.
  • PR Approver must confirm 📃 THIRD_PARTY_LICENSES.txt updated, if any 📚 library added/removed in pyproject.toml.

Copy link

📌 Cov diff with main:

Coverage-63%

📌 Overall coverage:

Coverage-62.23%

sphinx
sphinxcontrib-napoleon
sphinx_copybutton
sphinx_code_tabs
sphinx-autobuild
sphinx-autorun
oracle_ads==2.9.0rc0
oracle_ads
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this line?

Copy link
Member

Choose a reason for hiding this comment

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

This is needed for generation of class files for feature store for class level documentation

@@ -0,0 +1,110 @@
===============
Copy link
Member

Choose a reason for hiding this comment

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

This file will be removed?

Copy link
Member

Choose a reason for hiding this comment

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

yes. removed this file. thanks

if result.returncode == 0:
pass
else:
print(result.stderr)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the logger be used instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

done. thanks!1

ads/opctl/operator/common/backend_factory.py Show resolved Hide resolved
ads/opctl/operator/common/backend_factory.py Show resolved Hide resolved
Copy link

⚠️ This PR changed pyproject.toml file. ⚠️

  • PR Creator must update 📃 THIRD_PARTY_LICENSES.txt, if any 📚 library added/removed in pyproject.toml.
  • PR Approver must confirm 📃 THIRD_PARTY_LICENSES.txt updated, if any 📚 library added/removed in pyproject.toml.

Copy link

📌 Cov diff with main:

Coverage-1%

📌 Overall coverage:

Coverage-23.19%

Copy link

⚠️ This PR changed pyproject.toml file. ⚠️

  • PR Creator must update 📃 THIRD_PARTY_LICENSES.txt, if any 📚 library added/removed in pyproject.toml.
  • PR Approver must confirm 📃 THIRD_PARTY_LICENSES.txt updated, if any 📚 library added/removed in pyproject.toml.

Copy link

📌 Cov diff with main:

Coverage-1%

📌 Overall coverage:

Coverage-23.19%

Copy link

⚠️ This PR changed pyproject.toml file. ⚠️

  • PR Creator must update 📃 THIRD_PARTY_LICENSES.txt, if any 📚 library added/removed in pyproject.toml.
  • PR Approver must confirm 📃 THIRD_PARTY_LICENSES.txt updated, if any 📚 library added/removed in pyproject.toml.

Copy link

📌 Cov diff with main:

Coverage-1%

📌 Overall coverage:

Coverage-23.19%

Copy link

⚠️ This PR changed pyproject.toml file. ⚠️

  • PR Creator must update 📃 THIRD_PARTY_LICENSES.txt, if any 📚 library added/removed in pyproject.toml.
  • PR Approver must confirm 📃 THIRD_PARTY_LICENSES.txt updated, if any 📚 library added/removed in pyproject.toml.

Copy link

📌 Cov diff with main:

Coverage-1%

📌 Overall coverage:

Coverage-23.19%

pyproject.toml Outdated
@@ -190,10 +194,11 @@ pii = [
"spacy==3.6.1",
]
llm = [
"langchain>=0.0.295",
"langchain>=0.0.295,<=0.0.350",
Copy link
Member

Choose a reason for hiding this comment

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

If you restrict langchain<=0.0.350, you will also need langchain_community<=0.0.14 to pass the unit tests. Are you planning to remove this restriction in the future?

Copy link

⚠️ This PR changed pyproject.toml file. ⚠️

  • PR Creator must update 📃 THIRD_PARTY_LICENSES.txt, if any 📚 library added/removed in pyproject.toml.
  • PR Approver must confirm 📃 THIRD_PARTY_LICENSES.txt updated, if any 📚 library added/removed in pyproject.toml.

@@ -48,12 +51,13 @@ class OCIClientFactory:
oc.OCIClientFactory(**auth).object_storage # Creates Object storage client using instance principal authentication
"""

def __init__(self, config={}, signer=None, client_kwargs=None):
def __init__(self, config=(), signer=None, client_kwargs=None):
Copy link
Member

Choose a reason for hiding this comment

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

why using () as default here? I think we need {} as default. Otherwise there might be an error in some cases when using resource principal. The following example works in the existing ADS:

>>> import ads
>>> ads.set_auth("resource_principal")
>>> import oci
>>> signer = signer = oci.auth.signers.get_resource_principals_signer()
>>> from ads.common.oci_client import OCIClientFactory
>>> OCIClientFactory(signer=signer).object_storage
<oci.object_storage.object_storage_client.ObjectStorageClient object at 0x7fb04b094910>

but the same code will fail with this PR:

>>> import ads
>>> ads.set_auth("resource_principal")
>>> import oci
>>> signer = signer = oci.auth.signers.get_resource_principals_signer()
>>> from ads.common.oci_client import OCIClientFactory
>>> OCIClientFactory(signer=signer).object_storage
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/conda/lib/python3.8/site-packages/ads/common/oci_client.py", line 108, in object_storage
    return self.create_client("object_storage")
  File "/opt/conda/lib/python3.8/site-packages/ads/common/oci_client.py", line 104, in create_client
    return client(config=self.config, signer=self.signer, **kwargs)
  File "/opt/conda/lib/python3.8/site-packages/oci/object_storage/object_storage_client.py", line 119, in __init__
    self.base_client = BaseClient("object_storage", config, signer, object_storage_type_mapping, **base_client_init_kwargs)
  File "/opt/conda/lib/python3.8/site-packages/oci/base_client.py", line 304, in __init__
    endpoint=config.get('endpoint'),
AttributeError: 'tuple' object has no attribute 'get'

However probably it was a bad idea to put {} as default in the arguments. Maybe we should add some logic to set the default.

if not config:
    config = {}

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch!!!

Copy link

⚠️ This PR changed pyproject.toml file. ⚠️

  • PR Creator must update 📃 THIRD_PARTY_LICENSES.txt, if any 📚 library added/removed in pyproject.toml.
  • PR Approver must confirm 📃 THIRD_PARTY_LICENSES.txt updated, if any 📚 library added/removed in pyproject.toml.

Copy link

📌 Cov diff with main:

Coverage-63%

📌 Overall coverage:

Coverage-60.17%

Copy link
Member

@KshitizLohia KshitizLohia left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link

📌 Cov diff with main:

Coverage-63%

📌 Overall coverage:

Coverage-62.25%

@harsh97 harsh97 merged commit f04a4a6 into main Jan 24, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature store OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants