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

Make snorkel installable as a package #921

Merged
merged 14 commits into from
May 3, 2018

Conversation

dhimmel
Copy link
Contributor

@dhimmel dhimmel commented May 2, 2018

Closes #920.
Closes #918

Work in progress

Copy link
Contributor

@stephenbach stephenbach left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

@@ -1,4 +0,0 @@
[submodule "treedlib"]
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

.travis.yml Outdated

# Install all remaining dependencies as per our README
- pip install -r python-package-requirement.txt
- pip install .
- test -e parser/corenlp.sh || ./install-parser.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of CoreNLP installation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Done in f1dfee2

@@ -16,8 +16,7 @@
import warnings
warnings.filterwarnings("ignore", category=DeprecationWarning)

HOME = os.environ['SNORKELHOME']

HOME = os.path.abspath(os.path.join(__file__, '..', '..'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to move .html, etc. files into module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, let's do this in a follow up PR.

README.md Outdated
In both cases, the environment can be activated using `conda activate snorkel` and deactivated using `conda deactivate`.
With older versions of conda, you may have to replace `conda` with `source` in the above commands.

### Usage Environment
Copy link
Contributor

Choose a reason for hiding this comment

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

First instructions user should hit is how to install Snorkel as a user (dev environment can come lower in instructions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I added a quick start section in c1c0f27

- git+https://github.com/HazyResearch/snorkel@7eb7076f70078c06bef9752f22acf92fd86e616a
```
Finally, consider versioning the `numbskull` and `treedlib` pip dependencies by changing `master` to their latest commit hash on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

End of install instructions should emphatically point users to tutorials.

run.sh Outdated
@@ -2,9 +2,6 @@
source set_env.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this and set_env.sh? Right now this script installs CoreNLP, which we don't want to do automatically anymore, and launches jupyter.

@dhimmel dhimmel force-pushed the package branch 4 times, most recently from 722b670 to 0e0796c Compare May 3, 2018 20:10
Switch to absolute imports to ensure installation succeeded
@dhimmel dhimmel force-pushed the package branch 2 times, most recently from c1c0f27 to 7835e79 Compare May 3, 2018 20:46
@dhimmel dhimmel force-pushed the package branch 2 times, most recently from 51eaf31 to 9f15c9d Compare May 3, 2018 21:58
@stephenbach stephenbach changed the base branch from master to 0.7.0-alpha May 3, 2018 22:06
@dhimmel
Copy link
Contributor Author

dhimmel commented May 3, 2018

@ajratner would be great if you can take a look at this PR!

@dhimmel dhimmel force-pushed the package branch 2 times, most recently from 56458f0 to 070ce37 Compare May 3, 2018 22:28
@stephenbach stephenbach merged commit 70f6f24 into snorkel-team:0.7.0-alpha May 3, 2018
@dhimmel dhimmel mentioned this pull request May 30, 2018
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.

2 participants