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

build: improve build performance #386

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

baumstern
Copy link
Contributor

Description

This PR improve build performance:

  • container build time has improved by 4.5x (from 6m 31s down to 1m 31s)
  • image size of api package has decreased by 6.8x (from 1.58 GB to 232 MB)

These improvements were achieved by:

  • excluding unnecessary dependencies from other packages within this monorepo
  • build only target package, rather than building unrelated packages in the monorepo

Does this introduce a breaking change?

  • Yes
  • No

@ntampakas
Copy link
Collaborator

Hey @baumstern ,

i tried to execute docker compose with the changes, but the following error pops up on api side:

node:internal/modules/cjs/loader:1137
  throw err;
  ^

Error: Cannot find module 'ethers'
Require stack:
- /home/node/api/apps/node_modules/siwe/dist/client.js
- /home/node/api/apps/node_modules/siwe/dist/siwe.js
- /home/node/api/apps/api/dist/app/auth/auth.controller.js
- /home/node/api/apps/api/dist/app/auth/auth.module.js
- /home/node/api/apps/api/dist/app/app.module.js
- /home/node/api/apps/api/dist/main.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1134:15)
    at Module._load (node:internal/modules/cjs/loader:975:27)
    at Module.require (node:internal/modules/cjs/loader:1225:19)
    at require (node:internal/modules/helpers:177:18)
    at Object.<anonymous> (/home/node/api/apps/node_modules/siwe/dist/client.js:15:18)
    at Module._compile (node:internal/modules/cjs/loader:1356:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1414:10)
    at Module.load (node:internal/modules/cjs/loader:1197:32)
    at Module._load (node:internal/modules/cjs/loader:1013:12)
    at Module.require (node:internal/modules/cjs/loader:1225:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/node/api/apps/node_modules/siwe/dist/client.js',
    '/home/node/api/apps/node_modules/siwe/dist/siwe.js',
    '/home/node/api/apps/api/dist/app/auth/auth.controller.js',
    '/home/node/api/apps/api/dist/app/auth/auth.module.js',
    '/home/node/api/apps/api/dist/app/app.module.js',
    '/home/node/api/apps/api/dist/main.js'
  ] 
} 

Node.js v18.19.0

@baumstern baumstern mentioned this pull request Feb 14, 2024
2 tasks
@baumstern
Copy link
Contributor Author

Hey @baumstern ,

i tried to execute docker compose with the changes, but the following error pops up on api side:

node:internal/modules/cjs/loader:1137
  throw err;
  ^

Error: Cannot find module 'ethers'
Require stack:
- /home/node/api/apps/node_modules/siwe/dist/client.js
- /home/node/api/apps/node_modules/siwe/dist/siwe.js
- /home/node/api/apps/api/dist/app/auth/auth.controller.js
- /home/node/api/apps/api/dist/app/auth/auth.module.js
- /home/node/api/apps/api/dist/app/app.module.js
- /home/node/api/apps/api/dist/main.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1134:15)
    at Module._load (node:internal/modules/cjs/loader:975:27)
    at Module.require (node:internal/modules/cjs/loader:1225:19)
    at require (node:internal/modules/helpers:177:18)
    at Object.<anonymous> (/home/node/api/apps/node_modules/siwe/dist/client.js:15:18)
    at Module._compile (node:internal/modules/cjs/loader:1356:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1414:10)
    at Module.load (node:internal/modules/cjs/loader:1197:32)
    at Module._load (node:internal/modules/cjs/loader:1013:12)
    at Module.require (node:internal/modules/cjs/loader:1225:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/node/api/apps/node_modules/siwe/dist/client.js',
    '/home/node/api/apps/node_modules/siwe/dist/siwe.js',
    '/home/node/api/apps/api/dist/app/auth/auth.controller.js',
    '/home/node/api/apps/api/dist/app/auth/auth.module.js',
    '/home/node/api/apps/api/dist/app/app.module.js',
    '/home/node/api/apps/api/dist/main.js'
  ] 
} 

Node.js v18.19.0

This is due to missing peer dependency. I opened another PR to fix that: #390

@AronisAt79
Copy link

Thanks @baumstern ,

This is due to missing peer dependency. I opened another PR to fix that: #390

i understand the missing dependency issue was introduced with this PR #386 correct? If so, why not take care of it in the context of #386 as part of the review rounds? Would it be easier to follow up change history this way, instead of linking a redundant issue to be taken care of with a new PR?
I suggest all necessary changes that are addressing the build performance improvement and are required to make this PR functional should be included in #386.

@baumstern
Copy link
Contributor Author

baumstern commented Feb 14, 2024

Thanks @baumstern ,

This is due to missing peer dependency. I opened another PR to fix that: #390

i understand the missing dependency issue was introduced with this PR #386 correct? If so, why not take care of it in the context of #386 as part of the review rounds? Would it be easier to follow up change history this way, instead of linking a redundant issue to be taken care of with a new PR? I suggest all necessary changes that are addressing the build performance improvement and are required to make this PR functional should be included in #386.

Hi @AronisAt79 , the missing dependency issue is present in current codebase, it was not introduced by this PR. Thus I believe it's a separate concern. #389 has reproduction step.

@AronisAt79
Copy link

Thanks @baumstern ,

This is due to missing peer dependency. I opened another PR to fix that: #390

i understand the missing dependency issue was introduced with this PR #386 correct? If so, why not take care of it in the context of #386 as part of the review rounds? Would it be easier to follow up change history this way, instead of linking a redundant issue to be taken care of with a new PR? I suggest all necessary changes that are addressing the build performance improvement and are required to make this PR functional should be included in #386.

Hi @AronisAt79 , the missing dependency issue is present in current codebase, it was not introduced by this PR. Thus I believe it's a separate concern. #389 has reproduction step.

@baumstern i might be missing something here, did the api fail to start prior to 386?

@baumstern
Copy link
Contributor Author

baumstern commented Feb 15, 2024

Thanks @baumstern ,

This is due to missing peer dependency. I opened another PR to fix that: #390

i understand the missing dependency issue was introduced with this PR #386 correct? If so, why not take care of it in the context of #386 as part of the review rounds? Would it be easier to follow up change history this way, instead of linking a redundant issue to be taken care of with a new PR? I suggest all necessary changes that are addressing the build performance improvement and are required to make this PR functional should be included in #386.

Hi @AronisAt79 , the missing dependency issue is present in current codebase, it was not introduced by this PR. Thus I believe it's a separate concern. #389 has reproduction step.

@baumstern i might be missing something here, did the api fail to start prior to 386?

Thank you for your question, and I understand where the confusion might come from. The essence of the modifications introduced in this PR is to optimize our Docker build process. The strategy behind these changes is twofold:

  1. Selective Dependency Inclusion: This PR adjusts the build process to only include dependencies explicitly listed in the apps/api/package.json section, ensuring that only essential packages are incorporated into the container build

  2. Exclusion of Cross-Package Dependencies: It also ensures the exclusion of dependencies and devDependencies from other sub-packages within the monorepo (such as apps/client, apps/contracts, apps/dashboard, libs/api-sdk, libs/credentials, libs/hardhat, and libs/utils) that are not required for the api package

On another note, PR #390 is focused on addressing the misconfigured dependency graph across the monorepo. A notable issue is the misconfiguration involving ethers@5.5.1, which is we are concerned here, while declared as peerDependencies of siwe@^1.1.6, has not been specified as a dependency of the api package (It should specified under the dependencies and not under the devDependencies). This missing was indeed pre-existing and not a result of the modifications introduced in this PR.

Choosing to address the ethers dependency issue and other missing peerDependencies in a separate PR was aimed at ensuring changes remain focused and manageable. However, if consolidating this with the current PR is preferred for efficiency or clarity, I am willing to address in this PR together

@AronisAt79
Copy link

Thanks for the details @baumstern. I get then that the work done here caused the ethers dependency misconfig to surface, otherwise it went unnoticed since the api container had been working fine so far, is that correct? In that sense i think the separate PR is reasonable.

@0xjei
Copy link
Contributor

0xjei commented Feb 16, 2024

Thanks for the details @baumstern. I get then that the work done here caused the ethers dependency misconfig to surface, otherwise it went unnoticed since the api container had been working fine so far, is that correct? In that sense i think the separate PR is reasonable.

Thank you @baumstern and @AronisAt79 for the discussion. The #389 has been reviewed and ready for the merge. I will review this PR when the other has been merged successfully. Thank you and sorry for the delay!

@0xjei 0xjei changed the base branch from main to dev February 16, 2024 17:12
@0xjei
Copy link
Contributor

0xjei commented Feb 20, 2024

Thanks for the details @baumstern. I get then that the work done here caused the ethers dependency misconfig to surface, otherwise it went unnoticed since the api container had been working fine so far, is that correct? In that sense i think the separate PR is reasonable.

Thank you @baumstern and @AronisAt79 for the discussion. The #389 has been reviewed and ready for the merge. I will review this PR when the other has been merged successfully. Thank you and sorry for the delay!

We have merged the #390 (ref. #389). You can progress on this PR whenever. Thank you for your help @baumstern @AronisAt79

Copy link
Contributor

@0xjei 0xjei left a comment

Choose a reason for hiding this comment

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

Hello @baumstern! I got this error while running docker compose up -d. Noticed that I have removed the previous images / running container before running the compose.

giacomo@giacomo-tuxedo:~/Documents/EF/Code/bandada$ sudo docker compose up -d
[+] Running 18/18
 ✔ postgres 14 layers [⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿]      0B/0B      Pulled                                                                                                                    10.3s 
   ✔ e1caac4eb9d2 Pull complete                                                                                                                                                   1.6s 
   ✔ 7a2930f13d47 Pull complete                                                                                                                                                   1.2s 
   ✔ a6c49e965138 Pull complete                                                                                                                                                   0.9s 
   ✔ ed8dc94f857d Pull complete                                                                                                                                                   1.8s 
   ✔ 1f07b4807698 Pull complete                                                                                                                                                   2.4s 
   ✔ a776288d4030 Pull complete                                                                                                                                                   2.2s 
   ✔ 7cbb4adb3448 Pull complete                                                                                                                                                   2.3s 
   ✔ b6dbd7317d5f Pull complete                                                                                                                                                   2.7s 
   ✔ 52814b5dc710 Pull complete                                                                                                                                                   5.8s 
   ✔ b68697689b55 Pull complete                                                                                                                                                   2.9s 
   ✔ 6d80681e3923 Pull complete                                                                                                                                                   3.4s 
   ✔ 4270a9f40aee Pull complete                                                                                                                                                   3.5s 
   ✔ d28fa0286314 Pull complete                                                                                                                                                   4.0s 
   ✔ cb1ee5bc271e Pull complete                                                                                                                                                   4.2s 
 ! api Warning                                                                                                                                                                    2.4s 
 ! client Warning                                                                                                                                                                 2.4s 
 ! dashboard Warning                                                                                                                                                              2.4s 
[+] Building 44.8s (13/15)                                                                                                                                              docker:default
 => [api internal] load build definition from Dockerfile                                                                                                                          0.0s
 => => transferring dockerfile: 915B                                                                                                                                              0.0s
 => [api internal] load metadata for docker.io/library/node:18-alpine                                                                                                             1.9s
 => [api internal] load .dockerignore                                                                                                                                             0.0s
 => => transferring context: 628B                                                                                                                                                 0.0s
 => [api internal] load build context                                                                                                                                             0.1s
 => => transferring context: 6.60MB                                                                                                                                               0.1s
 => [api runner 1/4] FROM docker.io/library/node:18-alpine@sha256:ca9f6cb0466f9638e59e0c249d335a07c867cd50c429b5c7830dda1bed584649                                                2.5s
 => => resolve docker.io/library/node:18-alpine@sha256:ca9f6cb0466f9638e59e0c249d335a07c867cd50c429b5c7830dda1bed584649                                                           0.0s
 => => sha256:affdf979bd8ec516bf189d451b8ac68dd50adc49adc4c4014963556c11efeda4 1.16kB / 1.16kB                                                                                    0.0s
 => => sha256:24d8fcd7167fb06e91dc7228311105013dc042f6875ff2528ff7a41c04770112 7.14kB / 7.14kB                                                                                    0.0s
 => => sha256:4abcf20661432fb2d719aaf90656f55c287f8ca915dc1c92ec14ff61e67fbaf8 3.41MB / 3.41MB                                                                                    0.4s
 => => sha256:e7ced292c644a1f7bc776dcc401164b67c9224f8592cc83b8c42e237668a0c7f 40.25MB / 40.25MB                                                                                  1.7s
 => => sha256:b32c0114bba5af3e85af37dbc23b1e026850aba590099b81bf75946327b3a9e8 2.34MB / 2.34MB                                                                                    0.5s
 => => sha256:ca9f6cb0466f9638e59e0c249d335a07c867cd50c429b5c7830dda1bed584649 1.43kB / 1.43kB                                                                                    0.0s
 => => extracting sha256:4abcf20661432fb2d719aaf90656f55c287f8ca915dc1c92ec14ff61e67fbaf8                                                                                         0.0s
 => => sha256:f3748d9674b0ca905fe23e1cb4ad0e49d6a605dbbfb9d0cf485f300a03f1eeff 450B / 450B                                                                                        0.5s
 => => extracting sha256:e7ced292c644a1f7bc776dcc401164b67c9224f8592cc83b8c42e237668a0c7f                                                                                         0.5s
 => => extracting sha256:b32c0114bba5af3e85af37dbc23b1e026850aba590099b81bf75946327b3a9e8                                                                                         0.0s
 => => extracting sha256:f3748d9674b0ca905fe23e1cb4ad0e49d6a605dbbfb9d0cf485f300a03f1eeff                                                                                         0.0s
 => [api builder 2/8] WORKDIR /builder                                                                                                                                            0.2s
 => [api runner 2/4] WORKDIR /home/node/api                                                                                                                                       0.2s
 => [api builder 3/8] COPY .yarn ./.yarn                                                                                                                                          0.1s
 => [api builder 4/8] COPY package.json yarn.lock .yarnrc.yml ./                                                                                                                  0.1s
 => [api builder 5/8] COPY apps/api/package.json ./apps/api/                                                                                                                      0.1s
 => [api builder 6/8] RUN yarn workspaces focus api                                                                                                                              36.4s
 => [api builder 7/8] COPY apps/api ./apps/api                                                                                                                                    0.1s 
 => ERROR [api builder 8/8] RUN yarn workspace api build &&     yarn workspaces focus api --production                                                                            3.3s 
------                                                                                                                                                                                 
 > [api builder 8/8] RUN yarn workspace api build &&     yarn workspaces focus api --production:                                                                                       
3.221 src/app/credentials/credentials.service.ts:4:5 - error TS2305: Module '"@bandada/credentials"' has no exported member 'BlockchainProvider'.
3.221 
3.221 4     BlockchainProvider,
3.221       ~~~~~~~~~~~~~~~~~~
3.221 src/app/credentials/credentials.service.ts:5:5 - error TS2724: '"@bandada/credentials"' has no exported member named 'Web2Provider'. Did you mean 'Provider'?
3.221 
3.221 5     Web2Provider,
3.221       ~~~~~~~~~~~~
3.221 src/app/credentials/credentials.service.ts:7:5 - error TS2305: Module '"@bandada/credentials"' has no exported member 'Web2Context'.
3.221 
3.221 7     Web2Context,
3.221       ~~~~~~~~~~~
3.221 src/app/credentials/credentials.service.ts:8:5 - error TS2305: Module '"@bandada/credentials"' has no exported member 'BlockchainContext'.
3.221 
3.221 8     BlockchainContext
3.221       ~~~~~~~~~~~~~~~~~
3.221 
3.221 Found 4 error(s).
3.221 
------
failed to solve: process "/bin/sh -c yarn workspace api build &&     yarn workspaces focus api --production" did not complete successfully: exit code: 1
giacomo@giacomo-tuxedo:~/Documents/EF/Code/bandada$

@baumstern
Copy link
Contributor Author

Hello @baumstern! I got this error while running docker compose up -d. Noticed that I have removed the previous images / running container before running the compose.

giacomo@giacomo-tuxedo:~/Documents/EF/Code/bandada$ sudo docker compose up -d
[+] Running 18/18
 ✔ postgres 14 layers [⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿⣿]      0B/0B      Pulled                                                                                                                    10.3s 
   ✔ e1caac4eb9d2 Pull complete                                                                                                                                                   1.6s 
   ✔ 7a2930f13d47 Pull complete                                                                                                                                                   1.2s 
   ✔ a6c49e965138 Pull complete                                                                                                                                                   0.9s 
   ✔ ed8dc94f857d Pull complete                                                                                                                                                   1.8s 
   ✔ 1f07b4807698 Pull complete                                                                                                                                                   2.4s 
   ✔ a776288d4030 Pull complete                                                                                                                                                   2.2s 
   ✔ 7cbb4adb3448 Pull complete                                                                                                                                                   2.3s 
   ✔ b6dbd7317d5f Pull complete                                                                                                                                                   2.7s 
   ✔ 52814b5dc710 Pull complete                                                                                                                                                   5.8s 
   ✔ b68697689b55 Pull complete                                                                                                                                                   2.9s 
   ✔ 6d80681e3923 Pull complete                                                                                                                                                   3.4s 
   ✔ 4270a9f40aee Pull complete                                                                                                                                                   3.5s 
   ✔ d28fa0286314 Pull complete                                                                                                                                                   4.0s 
   ✔ cb1ee5bc271e Pull complete                                                                                                                                                   4.2s 
 ! api Warning                                                                                                                                                                    2.4s 
 ! client Warning                                                                                                                                                                 2.4s 
 ! dashboard Warning                                                                                                                                                              2.4s 
[+] Building 44.8s (13/15)                                                                                                                                              docker:default
 => [api internal] load build definition from Dockerfile                                                                                                                          0.0s
 => => transferring dockerfile: 915B                                                                                                                                              0.0s
 => [api internal] load metadata for docker.io/library/node:18-alpine                                                                                                             1.9s
 => [api internal] load .dockerignore                                                                                                                                             0.0s
 => => transferring context: 628B                                                                                                                                                 0.0s
 => [api internal] load build context                                                                                                                                             0.1s
 => => transferring context: 6.60MB                                                                                                                                               0.1s
 => [api runner 1/4] FROM docker.io/library/node:18-alpine@sha256:ca9f6cb0466f9638e59e0c249d335a07c867cd50c429b5c7830dda1bed584649                                                2.5s
 => => resolve docker.io/library/node:18-alpine@sha256:ca9f6cb0466f9638e59e0c249d335a07c867cd50c429b5c7830dda1bed584649                                                           0.0s
 => => sha256:affdf979bd8ec516bf189d451b8ac68dd50adc49adc4c4014963556c11efeda4 1.16kB / 1.16kB                                                                                    0.0s
 => => sha256:24d8fcd7167fb06e91dc7228311105013dc042f6875ff2528ff7a41c04770112 7.14kB / 7.14kB                                                                                    0.0s
 => => sha256:4abcf20661432fb2d719aaf90656f55c287f8ca915dc1c92ec14ff61e67fbaf8 3.41MB / 3.41MB                                                                                    0.4s
 => => sha256:e7ced292c644a1f7bc776dcc401164b67c9224f8592cc83b8c42e237668a0c7f 40.25MB / 40.25MB                                                                                  1.7s
 => => sha256:b32c0114bba5af3e85af37dbc23b1e026850aba590099b81bf75946327b3a9e8 2.34MB / 2.34MB                                                                                    0.5s
 => => sha256:ca9f6cb0466f9638e59e0c249d335a07c867cd50c429b5c7830dda1bed584649 1.43kB / 1.43kB                                                                                    0.0s
 => => extracting sha256:4abcf20661432fb2d719aaf90656f55c287f8ca915dc1c92ec14ff61e67fbaf8                                                                                         0.0s
 => => sha256:f3748d9674b0ca905fe23e1cb4ad0e49d6a605dbbfb9d0cf485f300a03f1eeff 450B / 450B                                                                                        0.5s
 => => extracting sha256:e7ced292c644a1f7bc776dcc401164b67c9224f8592cc83b8c42e237668a0c7f                                                                                         0.5s
 => => extracting sha256:b32c0114bba5af3e85af37dbc23b1e026850aba590099b81bf75946327b3a9e8                                                                                         0.0s
 => => extracting sha256:f3748d9674b0ca905fe23e1cb4ad0e49d6a605dbbfb9d0cf485f300a03f1eeff                                                                                         0.0s
 => [api builder 2/8] WORKDIR /builder                                                                                                                                            0.2s
 => [api runner 2/4] WORKDIR /home/node/api                                                                                                                                       0.2s
 => [api builder 3/8] COPY .yarn ./.yarn                                                                                                                                          0.1s
 => [api builder 4/8] COPY package.json yarn.lock .yarnrc.yml ./                                                                                                                  0.1s
 => [api builder 5/8] COPY apps/api/package.json ./apps/api/                                                                                                                      0.1s
 => [api builder 6/8] RUN yarn workspaces focus api                                                                                                                              36.4s
 => [api builder 7/8] COPY apps/api ./apps/api                                                                                                                                    0.1s 
 => ERROR [api builder 8/8] RUN yarn workspace api build &&     yarn workspaces focus api --production                                                                            3.3s 
------                                                                                                                                                                                 
 > [api builder 8/8] RUN yarn workspace api build &&     yarn workspaces focus api --production:                                                                                       
3.221 src/app/credentials/credentials.service.ts:4:5 - error TS2305: Module '"@bandada/credentials"' has no exported member 'BlockchainProvider'.
3.221 
3.221 4     BlockchainProvider,
3.221       ~~~~~~~~~~~~~~~~~~
3.221 src/app/credentials/credentials.service.ts:5:5 - error TS2724: '"@bandada/credentials"' has no exported member named 'Web2Provider'. Did you mean 'Provider'?
3.221 
3.221 5     Web2Provider,
3.221       ~~~~~~~~~~~~
3.221 src/app/credentials/credentials.service.ts:7:5 - error TS2305: Module '"@bandada/credentials"' has no exported member 'Web2Context'.
3.221 
3.221 7     Web2Context,
3.221       ~~~~~~~~~~~
3.221 src/app/credentials/credentials.service.ts:8:5 - error TS2305: Module '"@bandada/credentials"' has no exported member 'BlockchainContext'.
3.221 
3.221 8     BlockchainContext
3.221       ~~~~~~~~~~~~~~~~~
3.221 
3.221 Found 4 error(s).
3.221 
------
failed to solve: process "/bin/sh -c yarn workspace api build &&     yarn workspaces focus api --production" did not complete successfully: exit code: 1
giacomo@giacomo-tuxedo:~/Documents/EF/Code/bandada$

I updated to build missing dependencies. Please take another look!

Copy link
Contributor

@0xjei 0xjei left a comment

Choose a reason for hiding this comment

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

LGTM +1

Copy link
Collaborator

@ntampakas ntampakas left a comment

Choose a reason for hiding this comment

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

LGTM!

@vplasencia vplasencia merged commit f59b123 into bandada-infra:dev Mar 4, 2024
2 checks passed
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.

5 participants