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

Gramhagen/lgbm scenario #680

Merged
merged 36 commits into from
Mar 29, 2019
Merged

Gramhagen/lgbm scenario #680

merged 36 commits into from
Mar 29, 2019

Conversation

gramhagen
Copy link
Collaborator

@gramhagen gramhagen commented Mar 25, 2019 •
edited by miguelgfierro
Loading

Description

Adding WIP for LightGBM Scenario

Related Issues

#524

Checklist:

  • My code follows the code style of this project, as detailed in our contribution guidelines.
  • I have added tests.
  • I have updated the documentation accordingly

@review-notebook-app
Copy link

Check out this pull request on ReviewNB: https://app.reviewnb.com/Microsoft/Recommenders/pull/680

Visit www.reviewnb.com to know how we simplify your Jupyter Notebook workflows.

@gramhagen gramhagen self-assigned this Mar 25, 2019
@gramhagen gramhagen assigned anargyri and unassigned gramhagen and anargyri Mar 25, 2019
@miguelgfierro miguelgfierro changed the base branch from master to staging March 25, 2019 15:45
@gramhagen gramhagen self-assigned this Mar 25, 2019
@gramhagen gramhagen requested a review from anargyri March 25, 2019 15:46
@miguelgfierro
Copy link
Collaborator

@gramhagen not sure if you noticed, you did the PR to master. I changed it to staging.

yueguoguo
yueguoguo reviewed Mar 26, 2019
View reviewed changes
Copy link
Collaborator

@yueguoguo yueguoguo left a comment

Choose a reason for hiding this comment

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

It looks like the notebook is more of an operationalization of LightGBM model. So would it better to put it under the folder of notebook/05_operationalize?

notebooks/02_model/mmlspark_lightgbm_criteo.ipynb Outdated Show resolved Hide resolved
notebooks/02_model/mmlspark_lightgbm_criteo.ipynb Outdated Show resolved Hide resolved
notebooks/02_model/mmlspark_lightgbm_criteo.ipynb Outdated Show resolved Hide resolved
notebooks/02_model/mmlspark_lightgbm_criteo.ipynb Outdated Show resolved Hide resolved
@miguelgfierro
Copy link
Collaborator

hey guys @yueguoguo @gramhagen @jreynolds01, just FYI we had this weird error that the test of this notebook was failing, @anargyri found it was a problem of the DSVM and we had to uset SPARK_HOME. Now in DevOps we have:

. /anaconda/etc/profile.d/conda.sh && \
conda activate reco_pyspark && \
unset SPARK_HOME && \
pytest tests/unit -m "notebooks and spark and not gpu" --junitxml=reports/test-unit.xml && \
conda deactivate

@gramhagen
Copy link
Collaborator Author

We should a as this to the troubleshooting section in setup. Also it seems like it would be better to set spark home to the correct location? I was thinking a script which created the correct activate and deactivate conda scripts would be useful. Particularly for spark.

@miguelgfierro
Copy link
Collaborator

yeah, @anargyri added to the setup instructions in the conda activate

miguelgfierro
miguelgfierro approved these changes Mar 29, 2019
View reviewed changes
Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

I think everything is fixed and it is in a good shape for merging

@miguelgfierro miguelgfierro merged commit 705cebd into staging Mar 29, 2019
@miguelgfierro miguelgfierro deleted the gramhagen/lgbm_scenario branch March 29, 2019 23:48
@miguelgfierro miguelgfierro mentioned this pull request Apr 2, 2019
Closed
yueguoguo pushed a commit that referenced this pull request Sep 9, 2019
@miguelgfierro
Merge pull request #680 from Microsoft/gramhagen/lgbm_scenario
b61db55 
Gramhagen/lgbm scenario
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anargyri anargyri anargyri approved these changes

@miguelgfierro miguelgfierro miguelgfierro approved these changes

@yueguoguo yueguoguo yueguoguo left review comments

@jreynolds01 jreynolds01 Awaiting requested review from jreynolds01

Assignees

@gramhagen gramhagen

Labels
None yet
Projects
None yet
Milestone
LightGBM Based Recommender Scenario
Development

Successfully merging this pull request may close these issues.
5 participants
@gramhagen @miguelgfierro @yueguoguo @anargyri @jreynolds01