-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Draft: TXT2KG
w/ hotpot_qa.py
and tech_qa.py
examples
#9846
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
Conversation
w/
hotpot_qa.py` example for precision estimationTXT2KG
w/ hotpot_qa.py
example for precision estimation
@Kh4L reviews welcome |
improve #9846 --------- Co-authored-by: riship <riship@nvidia.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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 few nitpicks, but LGTM!
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.
Great work! 🚀 I may have missed some past PRs that were already merged, but I'm sharing my thoughts here anyway :)
I think we might want to make sure that anything goes under torch_geometric/
is general enough, well documented, well tested so that it's reusable by users outside these example scripts. If these utils under torch_geometric/
are not general enough, users might end up copying the code into their own scripts instead of directly relying on torch_geometric/
.
If the intention is for these LLMs+GNNs additions to serve as reference implementations (that users can tweak as needed), it'd make more sense to include them as examples rather than integrating them into torch_geometric/
.
"I think we might want to make sure that anything goes under torch_geometric/ is general enough, well documented, well tested so that it's reusable by users outside these example scripts" I totally agree btw, this is still a draft. i aim to have full docstrings etc and polish this up once its ready, just wanted an initial review. i will circle back when it is fully ready still have further work to get done before im ready |
Codecov ReportAttention: Patch coverage is
❌ Your patch check has failed because the patch coverage (21.60%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #9846 +/- ##
==========================================
+ Coverage 85.86% 85.93% +0.06%
==========================================
Files 490 492 +2
Lines 32432 32621 +189
==========================================
+ Hits 27847 28032 +185
- Misses 4585 4589 +4 ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
…eometric into rebase-txt2kg
for more information, see https://pre-commit.ci
…eometric into rebase-txt2kg
for more information, see https://pre-commit.ci
…eometric into rebase-txt2kg
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…eometric into rebase-txt2kg
no major diff but no decrease either --------- Co-authored-by: riship <riship@nvidia.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
accidentally merged wrong pr into this closing, making new |
Successor to [9666](#9666), this: - ~~updates the documentation to show how to utilize GNN RAG and~~(now handled by separate branch) - updates WebQSP to help serve as a toy example for LargeGraphIndexer. - fixes issues with LargeGraphIndexer running out of memory by introducing a default batch size and multithreading ability ~~currently blocked by a bug that causes the g_retriever.py example to get 1% less accuracy.~~ Bug is due to a fp32 precision issue related to batch kernels in Huggingface's transformers. Performance difference is too inconsequential to require a fix. may also be the cause of low retrieval precision in #9846 --------- Co-authored-by: Zack Aristei <zaristei@zaristei-nvidia.client.nvidia.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Zachary Aristei <zaristei@gmail.com> Co-authored-by: Rishi Puri <puririshi98@berkeley.edu> Co-authored-by: Rishi Puri <riship@nvidia.com>
improve pyg-team#9846 --------- Co-authored-by: riship <riship@nvidia.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Successor to [9666](pyg-team#9666), this: - ~~updates the documentation to show how to utilize GNN RAG and~~(now handled by separate branch) - updates WebQSP to help serve as a toy example for LargeGraphIndexer. - fixes issues with LargeGraphIndexer running out of memory by introducing a default batch size and multithreading ability ~~currently blocked by a bug that causes the g_retriever.py example to get 1% less accuracy.~~ Bug is due to a fp32 precision issue related to batch kernels in Huggingface's transformers. Performance difference is too inconsequential to require a fix. may also be the cause of low retrieval precision in pyg-team#9846 --------- Co-authored-by: Zack Aristei <zaristei@zaristei-nvidia.client.nvidia.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Zachary Aristei <zaristei@gmail.com> Co-authored-by: Rishi Puri <puririshi98@berkeley.edu> Co-authored-by: Rishi Puri <riship@nvidia.com>
closed, new version: #9992