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

Example Datasets with Python Package #501

Merged
merged 7 commits into from
Aug 31, 2020
Merged

Conversation

varunagrawal
Copy link
Collaborator

@varunagrawal varunagrawal commented Aug 31, 2020

Updated the setup.py to include the example data files and datasets. Resolves #500.

Note that we now install the data to a common "gtsam_example_data" directory, since we need CMake to know the path of the directory when building the .so file. This is the only way I've found to break this circular dependency (setup.py depends on .so and .so depends on where setup.py installs the data), so any better suggestions or recommendations are welcome!

@varunagrawal varunagrawal added the python Related to python wrapper label Aug 31, 2020
@varunagrawal varunagrawal self-assigned this Aug 31, 2020
@ProfFan
Copy link
Collaborator

ProfFan commented Aug 31, 2020

I think this would not work for wheels. I propose we have a Python version of this function, which uses import site to get the folder

@varunagrawal
Copy link
Collaborator Author

I actually checked that and it works with the wheel format. I am double checking right now.

@varunagrawal
Copy link
Collaborator Author

Yup, on running python setup.py bdist_wheel, I see output for adding the data files to the binary. 🎉

@ProfFan
Copy link
Collaborator

ProfFan commented Aug 31, 2020

I mean we should not touch anything outside of the site-packages directory, per the standard guidelines for python packages.

@dellaert
Copy link
Member

site-packages directory

Indeed, @varunagrawal, does the code now refer to the site-packages? That would be powerful magic, and would be the only way it could work for people that use "pip install gtsam", right?

@varunagrawal
Copy link
Collaborator Author

It doesn't quite unfortunately, because the C++ function findExampleDataFile has no idea what the site-packages directory can be at compile time.

I have seen packages like nltk be a bit more relaxed when it comes to packaging data (they download the corpus data into the home directory) but I agree with @ProfFan that it's good to stick with standards to avoid code rot (amongst other things).

@varunagrawal
Copy link
Collaborator Author

I'll add the custom function in a bit.

@dellaert
Copy link
Member

@varunagrawal Yeah let's not be relaxed. If there is no c++ way, is there a python way as Fan suggested?

@varunagrawal
Copy link
Collaborator Author

@dellaert there is no C++ way without some crazy magic due to the circular dependency (and even then it could break).

I am creating the python version. Building the wheel as of this comment.

@varunagrawal
Copy link
Collaborator Author

@dellaert @ProfFan all done!

Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Cool!!

@varunagrawal varunagrawal merged commit 920eae8 into develop Aug 31, 2020
@varunagrawal varunagrawal deleted the feature/python-data branch August 31, 2020 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to python wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FindExampleDataFile in python wrapper does not work
3 participants