Skip to content

Refactor configurable options and add sphinx docs - #1174

Merged
feast-ci-bot merged 12 commits into
feast-dev:masterfrom
terryyylim:refactor-constants
Nov 19, 2020
Merged

Refactor configurable options and add sphinx docs#1174
feast-ci-bot merged 12 commits into
feast-dev:masterfrom
terryyylim:refactor-constants

Conversation

@terryyylim

@terryyylim terryyylim commented Nov 18, 2020

Copy link
Copy Markdown
Member

Signed-off-by: Terence terencelimxp@gmail.com

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
Comment thread sdk/python/feast/constants.py Outdated

@terryyylim terryyylim Nov 18, 2020

Copy link
Copy Markdown
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.

@woop woop Nov 18, 2020

Copy link
Copy Markdown
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?

@terryyylim terryyylim Nov 18, 2020

Copy link
Copy Markdown
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.

@woop woop Nov 18, 2020

Copy link
Copy Markdown
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.

@terryyylim terryyylim Nov 19, 2020

Copy link
Copy Markdown
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
Refactor constants and add sphinx docs
22838c8 
Signed-off-by: Terence <terencelimxp@gmail.com>
@terryyylim
Set default jobservice to none
3834444 
Signed-off-by: Terence <terencelimxp@gmail.com>
@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 <terencelimxp@gmail.com>
woop
woop reviewed Nov 18, 2020
View reviewed changes
Comment thread sdk/python/feast/cli.py Outdated
woop
woop reviewed Nov 18, 2020
View reviewed changes
Comment thread sdk/python/feast/client.py Outdated
woop
woop reviewed Nov 18, 2020
View reviewed changes
Comment thread sdk/python/feast/constants.py
@terryyylim
Some fixes
6022ddc 
Signed-off-by: Terence <terencelimxp@gmail.com>
@woop

woop commented Nov 18, 2020

Copy link
Copy Markdown
Member

I love this PR.

@terryyylim
Address comments
fe6f433 
Signed-off-by: Terence <terencelimxp@gmail.com>
woop
woop reviewed Nov 18, 2020
View reviewed changes
Comment thread sdk/python/feast/constants.py Outdated
@terryyylim
Address comments
d310f17 
Signed-off-by: Terence <terencelimxp@gmail.com>
woop
woop reviewed Nov 18, 2020
View reviewed changes
Comment thread sdk/python/feast/constants.py Outdated
#: Feast Spark Job ingestion jobs staging location
SPARK_STAGING_LOCATION: str = ""

#: Feast Spark Job ingestion jar file

@woop woop Nov 18, 2020

Copy link
Copy Markdown
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?

@terryyylim terryyylim Nov 18, 2020

Copy link
Copy Markdown
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.

@woop woop Nov 19, 2020

Copy link
Copy Markdown
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?

@pyalex pyalex Nov 19, 2020

Copy link
Copy Markdown
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
Comment thread sdk/python/feast/constants.py Outdated
#: Spark Job launcher
SPARK_LAUNCHER: str = "dataproc" # standalone, dataproc, emr

#: Feast Spark Job ingestion jobs staging location

@woop woop Nov 18, 2020

Copy link
Copy Markdown
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?

@terryyylim terryyylim Nov 18, 2020

Copy link
Copy Markdown
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
Comment thread 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

@woop woop Nov 18, 2020

Copy link
Copy Markdown
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?

@terryyylim terryyylim Nov 18, 2020

Copy link
Copy Markdown
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
Comment thread sdk/python/feast/constants.py Outdated
#: Enable user authentication to Feast Core
ENABLE_AUTH: str = "False"

#: Auth token for user authentication to Feast

@woop woop Nov 18, 2020

Copy link
Copy Markdown
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?

@terryyylim terryyylim Nov 18, 2020

Copy link
Copy Markdown
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
Comment thread sdk/python/feast/constants.py Outdated
#: Default StatsD port
STATSD_PORT: str = ""

#: IngestionJob DeadLetter Destination

@woop woop Nov 18, 2020

Copy link
Copy Markdown
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?

@terryyylim terryyylim Nov 18, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated.

@pyalex pyalex Nov 19, 2020

Copy link
Copy Markdown
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
Comment thread sdk/python/feast/constants.py Outdated
AUTH_PROVIDER: str = "google"

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

@woop woop Nov 18, 2020

Copy link
Copy Markdown
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?

@terryyylim terryyylim Nov 19, 2020

Copy link
Copy Markdown
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 <terencelimxp@gmail.com>
@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 <terencelimxp@gmail.com>
@terryyylim
Update docs
889c0cd 
Signed-off-by: Terence <terencelimxp@gmail.com>
@woop woop mentioned this pull request Nov 18, 2020
Merged
@terryyylim
Cleanup spark launcher constant
0cca6f8 
Signed-off-by: Terence <terencelimxp@gmail.com>
@terryyylim

terryyylim commented Nov 19, 2020

Copy link
Copy Markdown
Member Author

/retest

woop
woop reviewed Nov 19, 2020
View reviewed changes
Comment thread 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.

@woop woop Nov 19, 2020

Copy link
Copy Markdown
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"?

@terryyylim
Update constants and clarify docstring
cf06308 
Signed-off-by: Terence <terencelimxp@gmail.com>
@terryyylim
Polish docstrings
4d7be0e 
Signed-off-by: Terence <terencelimxp@gmail.com>
@feast-ci-bot

feast-ci-bot commented Nov 19, 2020

Copy link
Copy Markdown
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

Details 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

pyalex commented Nov 19, 2020

Copy link
Copy Markdown
Collaborator

/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 <terencelimxp@gmail.com>

* Set default jobservice to none

Signed-off-by: Terence <terencelimxp@gmail.com>

* Fix auth tests

Signed-off-by: Terence <terencelimxp@gmail.com>

* Some fixes

Signed-off-by: Terence <terencelimxp@gmail.com>

* Address comments

Signed-off-by: Terence <terencelimxp@gmail.com>

* Address comments

Signed-off-by: Terence <terencelimxp@gmail.com>

* Test none default

Signed-off-by: Terence <terencelimxp@gmail.com>

* Set default as none

Signed-off-by: Terence <terencelimxp@gmail.com>

* Update docs

Signed-off-by: Terence <terencelimxp@gmail.com>

* Cleanup spark launcher constant

Signed-off-by: Terence <terencelimxp@gmail.com>

* Update constants and clarify docstring

Signed-off-by: Terence <terencelimxp@gmail.com>

* Polish docstrings

Signed-off-by: Terence <terencelimxp@gmail.com>
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