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

Factor EvalDescriptor out of SamplesDescriptor, improve JSDoc type annotations and more #987

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

Conversation

andrei-apollo
Copy link

This PR contains multiple non-user-visible changes to Inspect view. It is mostly (but not entirely) a subset of #911.

The only substantial change is factoring EvalDescriptor out of SamplesDescriptor. The EvalDescriptor is computed once per eval and contains information that doesn't change with the selected score. The SamplesDescriptor is now focused on data that depends on the selected score. I find that this separation improves readability, it should make caching better, and it is required for the fancy filter which doesn't depend on the selected score.

This PR also contains a lot of new JSDoc type annotations and type fixes.

Finally, it adds a prettierrc to ensure that per-user prettier settings don't break formatting.


Bonus pro-tip.

With all type errors fixed, it is now possible to enable type checking globally (not just in the open files) in VSCode/Cursor. I did this locally by creating a VSCode workspace file, inspect-ai.code-workspace, with the following content:

{
  "folders": [
    {
      "path": "."
    }
  ],
  "settings": {
    "typescript.tsserver.experimental.enableProjectDiagnostics": true
  }
}

Unfortunately, there is catch. We still have type errors in the generated files, and the TS plugin doesn't allow to exclude paths from type checking. So now I'm permanently using this error filter in VSCode:

!dist/*,!ansi-output.js,!tsconfig.json

Even with the filter, “Problems” counter includes these phantom errors, which is annoying, but when I navigate to the “Problems” tab, I only see real errors. Personally I find this to be an improvement, because I like global type checking. But I understand that this setup is far from ideal, so I'm not suggesting we enable it by default until I find a better way to suppress errors in generated files.

@dragonstyle
Copy link
Collaborator

Thank you for this! I'll have a look in detail tomorrow morning EST!

@dragonstyle dragonstyle self-requested a review December 11, 2024 23:58
Copy link
Collaborator

@dragonstyle dragonstyle left a comment

Choose a reason for hiding this comment

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

A few suggestions - thanks again for the PR!

src/inspect_ai/_view/www/src/api/Types.mjs Outdated Show resolved Hide resolved
src/inspect_ai/_view/www/src/App.mjs Outdated Show resolved Hide resolved
@dragonstyle
Copy link
Collaborator

This looks great to me, thank you! We're in the process of shipping an update to pypi and I'd like to wait until that has shipped to merge this (so we can get a few days / weeks testing internally before it goes to pypi). I'll merge it once we clear that release!

@andrei-apollo
Copy link
Author

Sounds good to me.

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