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 mlpack version to __init__.py #2092

Merged
merged 3 commits into from Nov 25, 2019

Conversation

@KimSangYeon-DGU
Copy link
Member

KimSangYeon-DGU commented Nov 22, 2019

I edited src/mlpack/bindings/python/CMakeLists.txt to append the mlpack version to __init__.py. By doing that, we can check the mlpack version in Python code, using the command below. (However, although I tried to place the __version__ at the bottom of the file, I couldn't find any good way.)

[Result]
result2

[Image]
result

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Nov 22, 2019

Awesome, thanks for the quick fix! If you want __version__ to appear after all the imports, one way to do that would be to move the file(APPEND ...) to the bottom of src/mlpack/CMakeLists.txt. The binding CMake scripts are sometimes a little crazy, but what's happening there is that we have to recurse through src/mlpack/methods/... and call the add_python_binding() macro (defined in src/mlpack/bindings/python/CMakeLists.txt). That add_python_binding() macro actually adds the from .mlpack import <X> to __init__.py.

So, at the bottom of src/mlpack/CMakeLists.txt, all of the lines have been added to __init__.py, and therefore if you do the file(APPEND ...) there, it will end up below all the imports. As a bonus, the version string is already available in that block of code. :)

@piyush26c

This comment has been minimized.

Copy link

piyush26c commented Nov 23, 2019

Done @rcurtin 👍

@piyush26c

This comment has been minimized.

Copy link

piyush26c commented Nov 23, 2019

I have done all the required changes in the src/mlpack/CMakeLists.txt as mentioned by @rcurtin,
will it be liable if I open the pull request with an updated file...?

@piyush26c

This comment has been minimized.

Copy link

piyush26c commented Nov 23, 2019

I have done the required changes in src/mlpack/CMakeLists.txt @rcurtin @KimSangYeon-DGU , will it be liable to me to pull the request with an updated file?

@piyush26c

This comment has been minimized.

Copy link

piyush26c commented Nov 23, 2019

But when I tried to build it again after getting changes in src/mlpack/CMakeLists.txt it failed..

@KimSangYeon-DGU

This comment has been minimized.

Copy link
Member Author

KimSangYeon-DGU commented Nov 23, 2019

@rcurtin Thanks for the detailed description. It's really helpful, and I decided to close this PR for the new contributor :).

@piyush26c It's okay to open your PR, if you resolve the build issue :)

@piyush26c

This comment has been minimized.

Copy link

piyush26c commented Nov 23, 2019

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Nov 23, 2019

@KimSangYeon-DGU any chance we can stick with this one? I was hoping to get it merged quickly so that I can do a release this weekend. 👍

@KimSangYeon-DGU

This comment has been minimized.

Copy link
Member Author

KimSangYeon-DGU commented Nov 24, 2019

@rcurtin OK, I'll work on this right now.

@piyush26c Please check our conversation above. I think you can take another issue labeled 'good first issue'

@KimSangYeon-DGU KimSangYeon-DGU force-pushed the KimSangYeon-DGU:version branch from 7ca33d8 to 5e37825 Nov 24, 2019
@KimSangYeon-DGU

This comment has been minimized.

Copy link
Member Author

KimSangYeon-DGU commented Nov 24, 2019

Thanks, @rcurtin. __version__ was appended to the bottom of the __init__.py.

result3

@piyush26c

This comment has been minimized.

Copy link

piyush26c commented Nov 24, 2019

@rcurtin OK, I'll work on this right now.

@piyush26c Please check our conversation above. I think you can take another issue labeled 'good first issue'

@KimSangYeon-DGU okay 👍

Copy link
Member

rcurtin left a comment

Awesome, looks good to me. 👍

@mlpack-bot
mlpack-bot bot approved these changes Nov 25, 2019
Copy link

mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit 5d57aa7 into mlpack:master Nov 25, 2019
13 of 17 checks passed
13 of 17 checks passed
mlpack.mlpack #20191124.6 failed
Details
mlpack.mlpack (Linux Markdown) Linux Markdown failed
Details
mlpack.mlpack (Linux Python27) Linux Python27 failed
Details
mlpack.mlpack (Linux Python37) Linux Python37 failed
Details
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mlpack.mlpack (Linux Plain) Linux Plain succeeded
Details
mlpack.mlpack (Windows VS14 Plain) Windows VS14 Plain succeeded
Details
mlpack.mlpack (Windows VS15 Plain) Windows VS15 Plain succeeded
Details
mlpack.mlpack (Windows VS16 Plain) Windows VS16 Plain succeeded
Details
mlpack.mlpack (macOS Plain) macOS Plain succeeded
Details
mlpack.mlpack (macOS Python27) macOS Python27 succeeded
Details
mlpack.mlpack (macOS Python37) macOS Python37 succeeded
Details
@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Nov 25, 2019

Awesome, great to have this fixed, thanks @KimSangYeon-DGU! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.