[ENG-9044] Add manage command to resync preprint dois v1 - #11617
[ENG-9044] Add manage command to resync preprint dois v1#11617Vlad0n20 wants to merge 9 commits into
Conversation
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Looks good overall. In addition to my questions/comments:
- Can we add logs of the output of the logs for you local run?
- We should also work with CE to test this command with a copy of production DB.
Vlad0n20
changed the title
There was a problem hiding this comment.
Looks good 👍
- Make sure we test this against a copy of prod DB before running this on prod.
- If there are errors, need look further on why they fail, and probably another fix.
- Test if the default batch size is good, how long does it take, and also see how many times we need to repeat this.
- Make sure whoever uses this command knows that we need to repeatedly run this command.
| help=( | ||
| 'Maximum number of preprints to process per run (default: 500). ' | ||
| 'The command processes the first N eligible preprints and exits; ' | ||
| 're-run the command to continue with the next batch.' | ||
| ), |
There was a problem hiding this comment.
Oh, I see. This is an OK alternative to the loop I suggested. Just need to make sure whoever runs this command is aware of the fact that they need to keep running this command until none exists. Cc @adlius for your input on this.
e803a0c to
e1d4931
Compare
|
LGTM |
|
|
||
| user.date_last_logged_in = timezone.now() | ||
| user.external_identity[provider][provider_id] = 'VERIFIED' | ||
| if provider.lower() in OSFUser.SOCIAL_FIELDS: |
There was a problem hiding this comment.
Is this part of this ticket?
adlius
left a comment
adlius
left a comment
There was a problem hiding this comment.
So I have a couple of suggestions. Since Crossref has a limit of 10000 submissions from a single user, might as well set the default batch size to 1000 and no rate limit. Instead let's run the task every five minutes so that we can send about 12000 requests per hour.
Also there are some commits from other branches that is in this PR.
| assert len(user.system_tags) == 1 | ||
|
|
||
| tag = Tag.all_tags.get(name=tag_name, system=True) | ||
| tag = Tag.all_tags.get(name=tag_name.lower(), system=True) |
There was a problem hiding this comment.
Same here. Is this part of this ticket?
| return qs | ||
|
|
||
|
|
||
| def resync_preprint_dois_v1(dry_run=True, batch_size=500, rate_limit=100, provider_id=None): |
There was a problem hiding this comment.
The rate limit of 100 is perhaps to restrictive. According to conversations with Crossref, they have a limit of 10000 pending items for each user.
There was a problem hiding this comment.
A 500 batch size is also perhaps too small.
|
|
||
| if rate_limit and not record_number % rate_limit: | ||
| logger.info(f'Rate limit reached at {record_number} preprints, sleeping {RATE_LIMIT_SLEEP}s') | ||
| time.sleep(RATE_LIMIT_SLEEP) |
There was a problem hiding this comment.
I am afraid this would clog the celery worker when this is run as a celery beat task.
brianjgeiger
left a comment
brianjgeiger
left a comment
There was a problem hiding this comment.
Drive by:
- What queue are the tasks going onto? The default queue? We should probably ensure that they go on the Low Priority queue to keep from clogging the pipes too much.
- If we have a cron job that happens every five minutes, how are you going to run this in a
dry-runscenario to make sure it's going to work before the cron job runs? We might want to rethink our strategy here.
adlius
left a comment
adlius
left a comment
There was a problem hiding this comment.
Some issues found during local testing.
|
|
||
| minted_doi = preprint.get_identifier_value('doi') | ||
| doi = minted_doi or self.build_doi(preprint) | ||
| doi = doi_override or minted_doi or self.build_doi(preprint) |
There was a problem hiding this comment.
So I tried this locally against the prod db. It seems it is trying to mint the same unversioned doi twice, pointing to two different resource: one with a v1 appendage, the other without the v1 appendage. This seems to have something to do with the fact that minted_doi here is the unversioned doi, so that it would short circuit this line and never reach self.build_doi to get the versioned doi with the v1 appendage.
7 participants
Ticket
Purpose
Changes
Side Effects
QE Notes
CE Notes
Documentation