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.

Sign up for GitHub

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

Jump to bottom

Refactor configurable options and add sphinx docs #1174

Merged
merged 12 commits into from
Nov 19, 2020
Merged

Refactor configurable options and add sphinx docs #1174

merged 12 commits into from
Nov 19, 2020

Conversation

terryyylim
Copy link
Member

Signed-off-by: Terence [email protected]

What this PR does / why we need it:
Currently, constants used for Config class are declared separately from default values and the term is used interchangeably with config options, making it difficult to maintain as the number of configurable options increase.

This PR addresses the following:

  • Address constants which have different parameter name to be the same
    eg. CONFIG_SPARK_HISTORICAL_FEATURE_OUTPUT_FORMAT = "historical_feature_output_format" to
    CONFIG_HISTORICAL_FEATURE_OUTPUT_FORMAT = "historical_feature_output_format"
  • Add documentation for configurable options.
    image

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

terryyylim
terryyylim commented Nov 18, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated
#: Oauth token request url
OAUTH_TOKEN_REQUEST_URL: str = ""

MAX_WAIT_INTERVAL: str = "60"
Copy link
Member Author

Choose a reason for hiding this comment

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

Excluded docs for this on purpose since it's only used internally.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by internally?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only used by wait_retry_backoff. Although it can be configured for ingest method, not sure if we want to expose that.

Copy link
Member

Choose a reason for hiding this comment

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

It's just strange how we allow users to set it but we dont document it. I think it should be documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Shifted out of configurable options.

@terryyylim terryyylim force-pushed the refactor-constants branch 2 times, most recently from 2bc2037 to eddf9e8 Compare November 18, 2020 08:27
@terryyylim
Fix auth tests
fc5c87b 
Signed-off-by: Terence <[email protected]>
woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/cli.py Outdated Show resolved Hide resolved
woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/client.py Outdated Show resolved Hide resolved
@terryyylim
Some fixes
6022ddc 
Signed-off-by: Terence <[email protected]>
@woop
Copy link
Member

woop commented Nov 18, 2020

I love this PR.

@terryyylim
Address comments
fe6f433 
Signed-off-by: Terence <[email protected]>
woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated Show resolved Hide resolved
@terryyylim
Address comments
d310f17 
Signed-off-by: Terence <[email protected]>
woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated
#: Feast Spark Job ingestion jobs staging location
SPARK_STAGING_LOCATION: str = ""

#: Feast Spark Job ingestion jar file
Copy link
Member

Choose a reason for hiding this comment

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

Is this http only?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup I think so.

Copy link
Member

Choose a reason for hiding this comment

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

how do users know that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It currently depends on spark launcher: in dataproc http and gs is supported; in emr http and s3, in local mode http and file

woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated
#: Spark Job launcher
SPARK_LAUNCHER: str = "dataproc" # standalone, dataproc, emr

#: Feast Spark Job ingestion jobs staging location
Copy link
Member

Choose a reason for hiding this comment

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

What are the possible options?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/constants.py
#: Time to wait for historical feature requests before timing out.
BATCH_FEATURE_REQUEST_WAIT_TIME_SECONDS: str = "600"

#: Authentication Provider - Google OpenID/OAuth
Copy link
Member

Choose a reason for hiding this comment

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

What are the possible options?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated
#: Enable user authentication to Feast Core
ENABLE_AUTH: str = "False"

#: Auth token for user authentication to Feast
Copy link
Member

Choose a reason for hiding this comment

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

What kind of auth? What does this mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated
#: Default StatsD port
STATSD_PORT: str = ""

#: IngestionJob DeadLetter Destination
Copy link
Member

Choose a reason for hiding this comment

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

What are the possible options?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again gs supported by dataproc launcher, s3 by emr.
We do not include s3 support in dataproc launcher and vice versa

woop
woop reviewed Nov 18, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated
AUTH_PROVIDER: str = "google"

#: Spark Job launcher
SPARK_LAUNCHER: str = "dataproc" # standalone, dataproc, emr
Copy link
Member

Choose a reason for hiding this comment

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

Should we change this to standalone so that users arent forced to use dataproc by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Defaults to None. Users should set this on their own.

@terryyylim
Test none default
469f38c 
Signed-off-by: Terence <[email protected]>
@terryyylim terryyylim force-pushed the refactor-constants branch 2 times, most recently from e1b9676 to 6fce7e9 Compare November 18, 2020 14:06
@terryyylim
Set default as none
36bc636 
Signed-off-by: Terence <[email protected]>
@terryyylim
Update docs
889c0cd 
Signed-off-by: Terence <[email protected]>
@woop woop mentioned this pull request Nov 18, 2020
Merged
@terryyylim
Copy link
Member Author

/retest

woop
woop reviewed Nov 19, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated
class ConfigMeta(type):
"""
Class factory which customizes ConfigOptions class instantiation.
Specifically, setting its name to lowercase of capitalized variable.
Copy link
Member

Choose a reason for hiding this comment

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

setting its name what is "it"?

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pyalex, terryyylim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pyalex
Copy link
Collaborator

pyalex commented Nov 19, 2020

/lgtm

@feast-ci-bot feast-ci-bot merged commit 78cbd13 into feast-dev:master Nov 19, 2020
pyalex pushed a commit that referenced this pull request Nov 24, 2020
@terryyylim @pyalex
Refactor configurable options and add sphinx docs (#1174)
286cb05 
* Refactor constants and add sphinx docs

Signed-off-by: Terence <[email protected]>

* Set default jobservice to none

Signed-off-by: Terence <[email protected]>

* Fix auth tests

Signed-off-by: Terence <[email protected]>

* Some fixes

Signed-off-by: Terence <[email protected]>

* Address comments

Signed-off-by: Terence <[email protected]>

* Address comments

Signed-off-by: Terence <[email protected]>

* Test none default

Signed-off-by: Terence <[email protected]>

* Set default as none

Signed-off-by: Terence <[email protected]>

* Update docs

Signed-off-by: Terence <[email protected]>

* Cleanup spark launcher constant

Signed-off-by: Terence <[email protected]>

* Update constants and clarify docstring

Signed-off-by: Terence <[email protected]>

* Polish docstrings

Signed-off-by: Terence <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woop woop woop left review comments

@pyalex pyalex pyalex approved these changes

@davidheryanto davidheryanto Awaiting requested review from davidheryanto

@khorshuheng khorshuheng Awaiting requested review from khorshuheng

@zhilingc zhilingc Awaiting requested review from zhilingc

Assignees

@pyalex pyalex

Labels
approved kind/housekeeping lgtm release-note-none size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@terryyylim @woop @feast-ci-bot @pyalex