-
Notifications
You must be signed in to change notification settings - Fork 13
Modifications for Classifier Pipeline #189
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
base: master
Are you sure you want to change the base?
Conversation
Revert to no scix_id
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions but I think the core of the PR is in good shape.
batch_list = [] | ||
self.logger.info('request_classifier called with filename {}'.format(filename)) | ||
with open(filename, 'r') as f: | ||
reader = csv.DictReader(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might make more sense to move this to the run.py
so that way this code isn't being pulled into the celery workers.
adsmp/tasks.py
Outdated
status = app.get_msg_status(msg) | ||
logger.info(f'Message status: {status}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could probably become debug statements long term so we aren't flooding the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes made and committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference between the classifications
column and the collections
column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to use the name classifications
so it would not be confused with the existing SOLR collections
field. The later commit with classifications
was ment to fix the earlier one with collections
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this file changed? I am just a bit concerned because this alembic upgrade not matching the one that was used to upgrade the DB previously could pose an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was having an issue at one point so I added the if statement to check the database. I can revert it so it matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted file committed.
No description provided.