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

ISS-273 - System footprint #99

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

Conversation

shide1989
Copy link
Contributor

🚀 Pull Request Overview

What does this PR do? 🤔

add CLI System footprint

@shide1989 shide1989 changed the title System footprint CLI - System footprint Feb 10, 2025
src/utils/systemInfo.ts Outdated Show resolved Hide resolved
* Get the list of global packages installed on the system for mnetrics.
*/
function getGlobalPackages() {
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should expand it to brew, apt and a few other most popular package managers.
Python version would also be very useful

Copy link
Contributor

@zhuk-aa zhuk-aa Feb 10, 2025

Choose a reason for hiding this comment

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

What else could be useful to decrease the amount of iterations when it comes to CLI commands 2501 needs to run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% sure apt is very interesting tbh, but brew, python, php?, rust?,ruby? why not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we could have information from utils like the tree utility :
Screenshot 2025-02-12 at 10 11 17

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, tree could be interesting if we manage to:

  • list the main folders
  • cut the list down to X main folders
  • list full paths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apt : apt list --installed looks interesting indeed, I thought you wanted to have for ex the apt version
tree:

  • I use it often its handy, I've always had in mind to add it in the context somehow if relevant.
  • Since its a utility like JQ, we could code a function that would output the same kind of display. Its easy (already did in the past) and as bonus we could re-used the existing methods to retrieve workspace files to ignore common vendor folders etc. Which would output a righteously filtered list of files, with a preferably configured max depth.

wdyt ?

@shide1989 shide1989 self-assigned this Feb 12, 2025
@shide1989 shide1989 changed the title CLI - System footprint ISS-273 - System footprint Feb 12, 2025
@shide1989
Copy link
Contributor Author

@zhuk-aa I did a package manager call system that fetches user installed packages, since we dont need to know the common installed packages on the user's machine as the AI will surely know it already.

Using this approach gives less packages than output all the packages, which can lead to having 200+ items.
I also added a tree generation, its nice and neat.

Now it gives something like :

sysinfo {
  "sysInfo": {
    "platform": "darwin",
    "type": "Darwin",
    "release": "24.1.0",
    "arch": "arm64",
    "package_manager": "brew",
    "installed_packages": [
      "argocd",
      "automake",
      "aws-sam-cli",
      "aws/tap/copilot-cli",
      "awscli",
      "azure-cli",
      "base64",
      ...
      "sleepwatcher",
      "sqlmap",
      "terraform",
      "thefuck",
      "tree",
      "ubuntu/microk8s/microk8s",
      "wakeonlan",
      "wget",
      "yarn",
      "zig"
    ]
  },
  "nodeInfo": {
    "version": "v20.18.1",
    "config": {
      "target_defaults": {
        "cflags": [],
        "default_configuration": "Release",
        "defines": [
          "NODE_OPENSSL_CONF_NAME=nodejs_conf",
          "NODE_OPENSSL_HAS_QUIC",
          "ICU_NO_USER_DATA_OVERRIDE"
        ],
        "include_dirs": [],
        "libraries": []
      },
      "variables": {
        "host_arch": "arm64",
        "node_install_npm": true,
        "node_prefix": "/",
        "node_shared_cares": false,
        "node_shared_http_parser": false,
        "node_shared_libuv": false,
        "node_shared_openssl": false,
        "node_shared_zlib": false
      }
    },
    "packages": [
      "@2501-ai/[email protected]",
      "@2501-ai/[email protected]",
      "[email protected]",
      "[email protected]"
    ]
  }
}
workspaceFileTree 
.
└── express-app
    ├── .gitignore
    ├── app.js
    ├── package-lock.json
    └── package.json

wdyt ?

const entries = Array.from(node.children.values());
const lastIndex = entries.length - 1;

entries.forEach((child, index) => {
Copy link
Contributor

@zhuk-aa zhuk-aa Feb 12, 2025

Choose a reason for hiding this comment

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

  • my gut feeling: full paths would be a lot more clear for LLMs than this kind of visualization
  • generation logic would be simpler.
  • if later we'll need to change it to folders not nested inside the workspace it would be easier to do

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are definitely right, event Claude tells it.
Screenshot 2025-02-13 at 09 40 22

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding sysinfo adds around 800 tokens, and adding the filetree with relative path adds up another ~1500 tokens on engine for example:

src/validation/chatSchema.ts
src/validation/configurationsSchema.ts
src/validation/containerSchema.ts
src/validation/fileSchema.ts
src/validation/jobsSchema.ts
tsconfig.build.json
tsconfig.json
types/express.d.ts
types/stream.d.ts
vercel.json
[DEBUG][09:41:42.382] Final prompt tokens : 564 <- without any additions
[DEBUG][09:41:42.385] Final prompt with sysinfo tokens: 1313
[DEBUG][09:41:42.392] Final prompt with workspaceFileTree tokens: 2791

src/utils/systemInfo.ts Outdated Show resolved Hide resolved
src/utils/systemInfo.ts Outdated Show resolved Hide resolved
/**
* Collect system info while respecting privacy.
*/
export type SystemInfo = {
Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking: it would be better to keep this type loose. So that we could add or remove info without changes in Engine (and without thinking of backwards compatibility)

{
  system: string, // 'platform, type, release, arch' all in one string
  installed_packages: string[], // including node packages
  package_manager: string[], // we can skip adding it now if it's too much work
  nodeVersion: string
}

but let's chat about it

src/utils/types.ts Outdated Show resolved Hide resolved
@shide1989 shide1989 marked this pull request as ready for review February 13, 2025 16:37
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.

2 participants