Make into proper lib add additional dependencies and CLI #32
Conversation
ncoop57
changed the title
setup.py
Outdated
| 'PyPDF2 == 1.26.0', | ||
| 'beautifulsoup4 == 4.9.3', | ||
| 'fastcore == 1.4.2', | ||
| 'huggingface_hub == 0.6.0', |
There was a problem hiding this comment.
For clarification, your commit's goal is to make it into a lib but will it be also pushed to hugging face? And have you considered adding it to PyPI ?
( I see you're the author of #12 )
There was a problem hiding this comment.
The huggingface hub thing was not supposed to be there, I was playing around with some dependencies and forgot to remove that. As for uploading it pypi, I can do that, but I wasn't sure if @paulbricman you were interested in having the keys to the kingdom so to speak for managing the pypi package. If not, I can get it setup on PyPi
There was a problem hiding this comment.
In the meantime, can you double check the dependencies to make sure all are needed ? That means removing huggingface_hub to start with.
For the PyPi let's wait on @paulbricman's answer
|
@thiswillbeyourgithub I think I address all your comments. Lemme know what you think |
|
|
||
| if not file.exists(): | ||
| print("File not found!") | ||
| raise SystemExit() | ||
| else: | ||
| full_text = file.read_text() | ||
| full_text = file.read_text()[:1_000] |
There was a problem hiding this comment.
I think it would be best to reduce the size of the example text rather than silently reading only a portion don't you think ? Anyhow why do you feel this is necessary ? Is the text really that big ?
There was a problem hiding this comment.
I was doing it to get quick results since it is an example script. I think it would be easier for people understand the output. I agree, it would be better to reduce the example text than this since people might not look to closely at it.
There was a problem hiding this comment.
It's not that big so I'll go ahead and just undo this change here and leave the file alone
| with open(output_file.absolute(), "a") as of: | ||
| of.write(string) | ||
| auto.consume_var(full_text) | ||
| auto.to_json("output.json", prefix="") |
There was a problem hiding this comment.
Given that this a substantial change, can you confirrmed you have tested it ?
There was a problem hiding this comment.
yeah, I've tested it. The other way kept failing since it was made with an outdated API. I'm betting the other examples also don't work, but I haven't looked at those
| @@ -0,0 +1,39 @@ | |||
| from autocards.autocards import Autocards | |||
| from fastcore.script import * | |||
There was a problem hiding this comment.
For clarity, I would prefer avoiding star import if you don't mind.
| sys.path.append("../../.") | ||
| from autocards import Autocards | ||
| # import sys | ||
| # sys.path.append("../../.") |
There was a problem hiding this comment.
Why comment it instead of removing it?
|
Thanks a lot for the improvement! I added a few questions. |
…nltk punkt