sagemaker-python-sdk icon indicating copy to clipboard operation
sagemaker-python-sdk copied to clipboard

Feature Store methods are broken

Open Guillem96 opened this issue 3 years ago • 3 comments

Describe the bug When calling FeatureGroup.create (and other) method the SDK fails with the following exception:

Traceback (most recent call last):
  File "test.py", line 15, in <module>
    fg.create(
  File "/home/guillem/miniconda3/envs/jk-nb/lib/python3.8/site-packages/sagemaker/feature_store/feature_group.py", line 521, in create
    return self.sagemaker_session.create_feature_group(**create_feature_store_args)
TypeError: create_feature_group() missing 1 required positional argument: 'self'

To reproduce

import sagemaker
from sagemaker.feature_store.feature_group import FeatureDefinition, FeatureGroup, FeatureTypeEnum

sagemaker_sess = sagemaker.Session()

fds = [
    FeatureDefinition(feature_name="label", feature_type=FeatureTypeEnum.INTEGRAL),
    FeatureDefinition(feature_name="test", feature_type=FeatureTypeEnum.INTEGRAL),
    FeatureDefinition(feature_name="message_id", feature_type=FeatureTypeEnum.STRING),
    FeatureDefinition(feature_name="date", feature_type=FeatureTypeEnum.STRING),
]

fg = FeatureGroup(name="test-fs", feature_definitions=fds)
fg.create(
    f"s3://{sagemaker_sess.default_bucket()}/test-store",
    record_identifier_name="message_id",
    event_time_feature_name="date",
    role_arn=sagemaker.get_execution_role(sagemaker_sess),
)

System information A description of your system. Please provide:

  • SageMaker Python SDK version: 2.88.1
  • Python version: Python 3.8.12
  • CPU or GPU: CPU
  • Custom Docker image (Y/N): N

Guillem96 avatar May 02 '22 07:05 Guillem96

The SageMaker session needs to be passed in during init.

This works for me:

...
fg = FeatureGroup(name="test-fs", feature_definitions=fds, sagemaker_session=sagemaker_sess)
...

setu4993 avatar Jun 22 '22 07:06 setu4993

But then the sagemaker session shouldn’t be optional right?

Guillem96 avatar Jun 22 '22 08:06 Guillem96

I don't think so. The docs do not say it is optional.

The default value is the class sagemaker.session.Session, which is getting triggered in your example (I ran into the same a couple hours ago).

setu4993 avatar Jun 22 '22 09:06 setu4993

Right you are @setu4993 .

The offending code is here,

@attr.s
class FeatureGroup:
    name: str = attr.ib(factory=str) # Though unrelated to this issue, we could also add a validator here
    sagemaker_session: Session = attr.ib(default=Session) # Default is Session class, not an instance of it. 
    feature_definitions: Sequence[FeatureDefinition] = attr.ib(factory=list)

and should be updated to something like,

@attr.s
class FeatureGroup:
    name: str = attr.ib(factory=str, validator=min_len(1)) # validator will ensure name as provided
    sagemaker_session: Session = attr.ib(factory=Session) # Now it would create an instance of sagemaker Session
    feature_definitions: Sequence[FeatureDefinition] = attr.ib(factory=list)

psnilesh avatar Jan 30 '23 05:01 psnilesh

Fixed by https://github.com/aws/sagemaker-python-sdk/pull/3667

Thank you for reporting

martinRenou avatar Sep 27 '23 15:09 martinRenou