allenai / scholarphi

An interactive PDF reader.
Apache License 2.0
416 stars 52 forks source link

Deprecate partner subdomain #382

Closed stupendousC closed 1 year ago

stupendousC commented 1 year ago

also plz ignore the branch name gh35326_chi2021demo where it referenced the wrong parent ticket lol

TESTING 👍

stupendousC commented 1 year ago

Will absorb the totally cool code changes Sam suggested in PR #381 . Will also not leave s2-api.ts alone after all since the main owners are cool with it, it's not like anyone actually uses it anyway, but it's probably best to keep both branches as consistent with each other as possible?

ca16 commented 1 year ago

If you're excited to do the api changes too, feel free! Two things about that: 1) I wonder if it would be better to do a separate PR for data-processing code, and a separate PR for api code? What do you think? 2) I don't think I'm equipped to review the api changes. Sounds like maybe it doesn't matter for this case given the review on the PR for main, and the fact that I'm not sure anyone uses the chi-2021-demo branch's api code, but just wanted to make sure that was clear...

ca16 commented 1 year ago

Oh also! Still wondering about the testing plan for the data-processing changes...

stupendousC commented 1 year ago

Oh also! Still wondering about the testing plan for the data-processing changes...

Yup, will get on that! stay tuned

stupendousC commented 1 year ago
  1. I wonder if it would be better to do a separate PR for data-processing code, and a separate PR for api code? What do you think?
  2. I don't think I'm equipped to review the api changes. Sounds like maybe it doesn't matter for this case given the review on the PR for main, and the fact that I'm not sure anyone uses the chi-2021-demo branch's api code, but just wanted to make sure that was clear...

hmmm yeah, I see what you're saying. Ok i'll move the s2-api.ts changes to another PR so it'll be easier to figure out if anything goes wrong on the API side.

stupendousC commented 1 year ago
  1. How does the output produced from your runs on this branch compare to output for the same paper produced from a run on the chi-2021-demo branch as it is rn?
  2. Did running pytest go alright? (just a reminder there's also that TC job that will run it for you)
Current chi-2021-demo branch This branch, WITH API key This branch, WITHOUT API key
1. compare resulting citations.json 🎯 match 👍 match 👍
2A. pytest 🆗 🆗 🆗
2B. pytest --all locally fail but 🆗 fail but 🆗 fail but 🆗
2C. pytest --all via TC 🆗 WIP <--