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

feat: use a local static cache for commonly used contexts #2587

Merged

Conversation

chumbert
Copy link
Contributor

@chumbert chumbert commented Nov 2, 2023

Related to this issue

  • Package a list of commonly used contexts
  • Replace the context loader from pyld with StaticCacheJsonLdDownloader
  • StaticCacheJsonLdDownloader resolves contexts from local package in priority, then delegates to a downloader
  • As of now, JsonLdDocumentDownloader and JsonLdDocumentParser are simply lifted from pyld in a way that decouples downloading and parsing JSON-LD documents.
  • Tests are mostly drafts, waiting for feedback before investing time there.

@swcurran
Copy link
Contributor

swcurran commented Nov 2, 2023

Could you explain the mechanics here? I’m trying to see the connection between the locally stored file and the URL.

How do we detect changes in the static files — at least at build time? I’m assuming that is a concern.

Presumably, any tests would fail the build time tests, which is a pain, but better than failing in a production environment :-).

This is good stuff — thanks.

@chumbert
Copy link
Contributor Author

chumbert commented Nov 2, 2023

Could you explain the mechanics here? I’m trying to see the connection between the locally stored file and the URL.

Of course, sorry for the sparse PR description.

In essence, the new loader that replaces the pyld load checks contexts URLs against a mapping from URL to already loaded documents:

  • StaticCacheJsonLdDownloader holds the mapping for known contexts.
  • Loading documents from filesystem happens at startup when the conductor binds the DocumentLoader instance.
  • This loading is implemented within the StaticCacheJsonLdDownloader constructor using the _load_jsonld_file function.
  • Static contexts are bundled within aca-py python package and are stored in the project filestructure (you can see them in aries_cloudagent/vc/ld_proofs/resources.
  • When aca-py tries to resolve a context, an URL present in the cache results in an immediate response, others go through the code that existed previously

How do we detect changes in the static files — at least at build time? I’m assuming that is a concern.

We don't and this is the pain of having files statically in the build. We'd need to keep contexts up-to-date. I'm not sure this is a big concern given that contexts are supposed to be versioned rather than changed as far as I understand. The medium term solution for aca-py would be to have the document loader be able to pick contexts from some configuration so that the burden of maintaining this list of known contexts is not solely on aca-py maintainers and contributors.

Presumably, any tests would fail the build time tests, which is a pain, but better than failing in a production environment :-).

As the files are present within the projects code, build time is unchanged, the only effect is that the package is marginally larger. I might be missing your point here.

@chumbert chumbert force-pushed the feature/static-context-cache branch 2 times, most recently from c829eab to 9998e91 Compare November 6, 2023 06:45
@swcurran
Copy link
Contributor

swcurran commented Nov 6, 2023

Thanks for the overview. Let us know when you are happy with this one for a review/merge.

Perhaps the “maintenance” question is a GHA that runs either only when triggered or perhaps on a cron job (once a month?) that checks the pre-loaded contexts against the static files and reports an error on a mismatch. It would need a good error message since it will happen so rarely :-).

@dbluhm dbluhm added the 0.11.0 label Nov 6, 2023
@dbluhm dbluhm added this to the 0.11.0 milestone Nov 6, 2023
@dbluhm dbluhm marked this pull request as ready for review November 8, 2023 03:06
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this an in depth look, made some very minor adjustments as recommended by pyright, and I'm happy with this. There's opportunity for more sophisticated stuff in the future but I think this gets us more than 90% of the way there.

I wish pyld was a little more... idiomatic python but I don't want to write anything to replace it lol. It does some things that are a little odd, especially next to other code in ACA-Py, but it's not too bad.

@dbluhm
Copy link
Contributor

dbluhm commented Nov 8, 2023

@swcurran for the sake of transparency, I'll let you do the honors of merging this PR since I made some small contributions to it.

@swcurran
Copy link
Contributor

swcurran commented Nov 8, 2023

I did the merge in the wrong order, so this needs an update to main before it can be merged. Sorry about that.

chumbert and others added 3 commits November 8, 2023 15:36
Because they make me feel better even though pyld doesn't care lol

Signed-off-by: Daniel Bluhm <[email protected]>
@chumbert chumbert force-pushed the feature/static-context-cache branch from 814e40f to 52db475 Compare November 8, 2023 14:36
Copy link

sonarqubecloud bot commented Nov 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dbluhm
Copy link
Contributor

dbluhm commented Nov 8, 2023

I did the merge in the wrong order, so this needs an update to main before it can be merged. Sorry about that.

I'll consider this an indication of intent to merge and will go ahead and push the button now that this branch is up to date and tests are passing!

@dbluhm dbluhm merged commit 5ff523c into openwallet-foundation:main Nov 8, 2023
9 checks passed
@swcurran swcurran removed the 0.11.0 label Nov 8, 2023
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.

3 participants