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

Add benchmark testing apis #66

Merged
merged 43 commits into from
Dec 3, 2020
Merged

Add benchmark testing apis #66

merged 43 commits into from
Dec 3, 2020

Conversation

ChengFR
Copy link
Collaborator

@ChengFR ChengFR commented Nov 30, 2020

The following tasks have been accomplished:

sarahmish and others added 30 commits October 15, 2020 22:36
@CLAassistant
Copy link

CLAassistant commented Nov 30, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@sarahmish sarahmish left a comment

Choose a reason for hiding this comment

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

Looks great @ChengFR! I have some minor comments, if you would kindly address them.

I also want to leave out the python notebooks that are on the other branch (1, 2, 3, and 4) from this PR, and keep it dedicated to benchmarking (benchmarking and tasks can be left for illustration usage purposes).

cardea/benchmark/benchmark.py Outdated Show resolved Hide resolved
cardea/benchmark/benchmark.py Show resolved Hide resolved
cardea/benchmark/benchmark.py Outdated Show resolved Hide resolved
cardea/benchmark/benchmark.py Outdated Show resolved Hide resolved
cardea/benchmark/benchmark.py Outdated Show resolved Hide resolved
raise TypeError("Unsupported file type {}.".format(extension))
pipeline.set_hyperparameters(init_hyperparameters)

# Load Dataset.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Load/ Construct Dataset. In the future, the "data_loader" and "problem_definition" will construct the feature matrix. I am thinking we can make feature_matrix a more generic name like, dataset. which can be used for all stages to generate the feature matrix.

Dataset can be:

  • feature matrix (pandas.DataFrame).
  • entityset (Featuretools.EntitySet).
  • raw data (list of pandas.DataFrame).

This is different than the consensus we reached in the design document, but is more generic.. Otherwise always assume I am reading my data from a path_to_dataset from the task itself.

We can proceed without doing any changes, but I am just marking them here in case we refer back to this idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The feature_matrix in the inputs of evaluate_task is a reserved parameter. When we run multiple pipelines on the same dataset from an entityset or raw data, we don't want to run the featurization multiple times. In this case, the upper-level function benchmark will do the featurization and pass the feature_matrix to evaluate_task.

So I don't think it is necessary to make it more general.

cardea/benchmark/benchmark.py Outdated Show resolved Hide resolved
cardea/benchmark/benchmark.py Outdated Show resolved Hide resolved
Comment on lines +13 to +14
# Add customized primitives from a local source.
add_primitives_path(MLBLOCKS_PRIMITIVES)
Copy link
Collaborator

@sarahmish sarahmish Nov 30, 2020

Choose a reason for hiding this comment

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

Is this necessary? I tried running the pipeline directly and it was fine.

Our definition of MLBLOCKS_PRIMITIVES is in the entry point (view setup.py), so it should be working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that in my testing environment, without this sentence, the customized primitives cannot be found. Can you double-check this problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

After investigating this, the entry_point mentioned is only possible in the newer version of MLBlocks. We can keep this implementation for now, but mark it to be removed after the upgrade.

Comment on lines +13 to +14
PIPELINE_DIR = './cardea/pipelines'
VERIFIED_DIR = './benchmark/verified'
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use another approach (same as the reason earlier).

from import dirname

PIPELINE_DIR  = os.path.join(os.path.dirname(__file__), 'pipelines')
VERIFIED_DIR = os.path.join(os.path.dirname(os.path.dirname(__file__)), 'benchmark', 'verified')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Current paths in task configurations are all relative paths from the root. Since we will share the benchmarking results, this design ensures that the tasks are runnable on others' devices. Are you suggesting we use absolute paths instead (which is more readable but not runnable on different devices)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see your intention but the problem is this is difficult to trace. My suggestion would be to have the absolute path and the users should be following the same configuration of Cardea.

I imagine that PIPELINE_DIR will be defined in the same manner (in all cases) but the VERIFIED_DIR will change depending on the user. Is this the case you are referring to by

not runnable on different devices?

If that's what you mean, then I think that is not an issue, it can always be overriden by the user to specify their own path. But generally speaking, absolute paths are more maintainable and traceable as well.

Copy link
Collaborator

@sarahmish sarahmish left a comment

Choose a reason for hiding this comment

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

Looks good @ChengFR!

@ChengFR ChengFR merged commit 38987e9 into master Dec 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modeler load pipelines instead of lists of primitives Benchmark testing apis Primitive setup
3 participants