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

Load index #59

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Load index #59

wants to merge 7 commits into from

Conversation

Anwesh1
Copy link
Contributor

@Anwesh1 Anwesh1 commented Mar 10, 2021

Overview

I think this allows users to pass in a file path for an already existing index on their drive.

I tried testing it by having the program write the index to a file and on the next run I passed in the file path for that file and it seemed to work. When it loaded from an index I could see the significant speed boost instead of the original method.

Let me know if you guys see any issues, otherwise, we can merge.
PS: There are 2 commits, the first one has all the code updates, the second one was a minor flake8 fix.

Closes

Closes #56.

semantic_search/main.py Outdated Show resolved Hide resolved
semantic_search/main.py Outdated Show resolved Hide resolved
semantic_search/main.py Outdated Show resolved Hide resolved
semantic_search/main.py Outdated Show resolved Hide resolved
Anwesh1 and others added 4 commits March 10, 2021 14:44
@Anwesh1
Copy link
Contributor Author

Anwesh1 commented Mar 10, 2021

Thanks for the input @JohnGiorgi, is there anything else required?

semantic_search/main.py Outdated Show resolved Hide resolved
Co-authored-by: John Giorgi <[email protected]>
@JohnGiorgi JohnGiorgi changed the base branch from master to develop March 10, 2021 19:50
@JohnGiorgi
Copy link
Collaborator

JohnGiorgi commented Mar 10, 2021

@Anwesh1 Looks good.

One thing, I changed the base branch from master to develop. The reason being that we are moving fast here (good) but that increases the chances we will break something. Let's keep master clean for now. At some point, we can merge develop into master.

Otherwise, LGTM.

@Anwesh1
Copy link
Contributor Author

Anwesh1 commented Mar 10, 2021

Sure, sounds good. Thanks, @JohnGiorgi.
Will merge after all the tests.

@JohnGiorgi
Copy link
Collaborator

@Anwesh1 Actually, it would be great if we could add a test to see if the server works as expected when an index is loaded from disk. Think you can handle that or would you rather I take it?

@Anwesh1
Copy link
Contributor Author

Anwesh1 commented Mar 10, 2021

I'm not sure how to go about doing that. Do you think you could show me sometime?
Would this be a pytest test?

@JohnGiorgi
Copy link
Collaborator

@Anwesh1 No problem, it is rather complicated so I will take it.

It would be good (for this project and beyond) to start getting familiar with pytest, though. They have great documentation, and there are lots of tutorials online. Unit testing is an important part of software engineering and pytest is the tool of choice of unit testing python code.

@Anwesh1
Copy link
Contributor Author

Anwesh1 commented Mar 10, 2021

Okay sounds good, I'll definitely do that. Sure go ahead and tackle it when you can and then feel free to merge

@Anwesh1
Copy link
Contributor Author

Anwesh1 commented Mar 12, 2021

Importing @jvwong code from pubmed-dl for the ncbi.py file works perfectly. However, the only issue is that one of the pytest tests is failing. Otherwise, the code works as intended.

image

@JohnGiorgi
Copy link
Collaborator

@Anwesh1 Can you do the copy and paste in a new (fresh) branch off of develop, and open a PR? We can discuss this issue there.

Base automatically changed from develop to master May 19, 2021 20:25
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.

Load the index from disk
2 participants