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

Fix Python Dockerfile #5984

Merged
merged 1 commit into from Jul 19, 2023

Conversation

GyuminJack
Copy link
Contributor

dockerfile is updated 2 month ago.

there is missed change directory command. (it is originally existed)

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to contribute!

Please see one suggestion I left. In the future, we'd appreciate if you tested your changes prior to submitting them.

@@ -26,7 +26,7 @@ RUN apt-get update && \
# lightgbm
conda install -q -y numpy scipy scikit-learn pandas && \
git clone --recursive --branch stable --depth 1 https://github.com/Microsoft/LightGBM && \
sh ./build-python.sh install && \
cd LigthGBM && sh ./build-python.sh install && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cd LigthGBM && sh ./build-python.sh install && \
cd ./LightGBM && \
sh ./build-python.sh install && \

Please put this change on a separate line, for consistency with the rest of this Dockerfile (which is done to make it easier to see the sequence of commands).

And please note that it's LightGBM, not LihgtGBM.

@jameslamb jameslamb changed the title [MISC] FIx Dockerfile Fix Python Dockerfile Jul 17, 2023
@GyuminJack
Copy link
Contributor Author

I misspelled it when I typed it in the source after testing it on something else.
I apologize. (i tested twice this time..😅)

Next time I contribute, I'll be sure to test or build against the changed source and open a PR.

Thanks for the thoughtful comment.

@jameslamb
Copy link
Collaborator

No problem, thanks for the fix!

By the way, please check you gitconfig settings. It looks like you're pushing commits here that aren't signed with an email tied to your GitHub account. That means GitHub might not include you as a "contributor" in this project's statistics, and people won't be able to click through to your account from the commit log or git blame view.

See #5532 (comment) for some details on what I mean.

@GyuminJack
Copy link
Contributor Author

i did below code. ( ref, uptake/pkgnet#284)

git config --global user.email "my email"
git reset --soft HEAD~2
git commit -m "fix commit message"
git push origin master --force

Is this what you were expecting?

@jameslamb
Copy link
Collaborator

Looks like that fixed it! The commits are now tied to your GitHub.

Screen Shot 2023-07-17 at 8 15 43 PM

Is this what you were expecting?

It doesn't matter to me personally, we could have merged this either way. Just want to be sure you get credit for your contribution 😊

@jameslamb jameslamb merged commit 8b1ab4f into microsoft:master Jul 19, 2023
41 checks passed
@jameslamb
Copy link
Collaborator

thanks!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants