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

Deno hydrate using deno cache #585

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

hicksy
Copy link

@hicksy hicksy commented May 27, 2021

Thank you for helping out! ✨

Hi 👋

This PR would work alongside this one in Hydrate - architect/hydrate#136

Together they enable hydration for deno runtime arc functions by setting the DENO_DIR to vendor/.deno_cache within each lambda and using deno cache to pre-cache the remote dependencies

This massively speeds up individual invocation and stops issues where the lambda times out while Deno is caching the remote dependencies.

Also could prove to be a handy pre-cache that could be used by the Deno lambda layer, moving the .deno_cache to /tmp to deploy pre-cached / hydrated Deno functions

We really appreciate your commitment to improving Architect

To maintain a high standard of quality in our releases, before merging every pull request we ask that you've completed the following:

  • Forked the repo and created your branch from master
  • Made sure tests pass (run npm it from the repo root)
  • Expanded test coverage related to your changes:
    • Added and/or updated unit tests (if appropriate)
    • Added and/or updated integration tests (if appropriate)
  • Updated relevant documentation:
  • Summarized your changes in changelog.md
  • Linked to any related issues, PRs, etc. below that may relate to, consume, or necessitate these changes

Please also be sure to completed the CLA (if you haven't already).

Learn more about contributing to Architect here.

Thanks again!

@@ -91,6 +91,18 @@ module.exports = function maybeHydrate (inventory, callback) {
}
else callback()
},
function _deno(callback) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this block needs to be linted

Copy link
Author

Choose a reason for hiding this comment

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

Sorry 👍

@@ -91,6 +91,18 @@ module.exports = function maybeHydrate (inventory, callback) {
}
else callback()
},
function _deno(callback) {
if(inv.lambdasBySrcDir[path] !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

This check is unnecessary, this function only got called on Lambdas that exist in the project inventory

Copy link
Author

Choose a reason for hiding this comment

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

Ah.. the tests fail without that block

/Users/martinhicks/Projects/sandbox/src/lib/maybe-hydrate.js:95
            let isDenoRuntime = (inv.lambdasBySrcDir[path].config.runtime === 'deno')
                                                           ^

TypeError: Cannot read property 'config' of undefined

Screenshot 2021-05-28 at 13 28 48

Copy link
Author

Choose a reason for hiding this comment

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

OK, happens when the path is the shared or views dir path. Have added that to the isDenoRuntime condition

@@ -91,6 +91,18 @@ module.exports = function maybeHydrate (inventory, callback) {
}
else callback()
},
function _deno(callback) {
if(inv.lambdasBySrcDir[path] !== undefined) {
let isDenoRuntime = (inv.lambdasBySrcDir[path].config.runtime === 'deno')
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the only check you need to run to install!

@ryanblock
Copy link
Member

Sorry this has lingered, this is an absolutely necessary problem for us to solve.

The current Deno runtime bootstrap is DENO_DIR=/tmp/deno_dir deno run..., as you mentioned assuming that DENO_DIR is in the writable filesystem. That's not tenable for the reasons you mentioned, as it's going to kill coldstart.

These PRs make sense to merge, but only once we can update that runtime, otherwise it won't be looking in the correct place. Any reason we should use ./vendor/.cache instead of the default $HOME/.cache/deno? Then we can just remove the DENO_DIR env var entirely.

Additionally: as I'm looking into it, our Deno runtime is extraordinarily old. Do you have a preferred target version you feel we should have Deno pinned at for a little while?

@hicksy
Copy link
Author

hicksy commented Aug 7, 2021

No probs.

our Deno runtime is extraordinarily old. Do you have a preferred target version you feel we should have Deno pinned at for a little while

Personally, I would pin to v1.10.3

There were a number of changes introduced to v1.11.x, which have a few breaking affecting dependencies to Deno's stdlib 0.97... (This affects functions-deno, which I've almost revolved now that the 3rd party AWS lib has been updated)

Any reason we should use ./vendor/.cache instead of the default $HOME/.cache/deno? Then we can just remove the DENO_DIR env var entirely.

If you mean locally in sandbox, then doing so would give you the benefit of Deno's cache, but it wouldn't help with the remote function coldstart as your cache would be in $HOME/.cache/deno and therefore outside of the deployed directory.

Using ./vendor/.cache each function's discreet cache would be deployed, allowing you to move the cache to /tmp/deno_dir prior to invoking the lambda layer's bootstrap DENO_DIR=/tmp/deno_dir deno run

If we didn't have it in ./vendor/.cache the cache would have to be rebuilt on each coldstart invocation

Does that help? sorry if I've misunderstood the question

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants