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

cli profile improvements #8223

Merged
merged 4 commits into from
May 23, 2024
Merged

cli profile improvements #8223

merged 4 commits into from
May 23, 2024

Conversation

wardi
Copy link
Contributor

@wardi wardi commented May 13, 2024

Fixes #

Proposed fixes:

  • ckan profile profile /url should be spelled ckan profile /url
  • don't profile first run by default
  • take profile from best-of-n runs (3 by default)

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport (first commit)

@wardi wardi changed the title cli profile command has too many profiles cli profile improvements May 14, 2024
@wardi
Copy link
Contributor Author

wardi commented May 14, 2024

@smotornyuk my pyright errors don't look like the ones from CI. It is using the pyproject.toml settings so there must be something else I'm missing, here are some of the errors I see locally (I don't see the ones reported above):

/home/ian/cd/ckan-docker/src/ckan/ckan/cli/shell.py
  /home/ian/cd/ckan-docker/src/ckan/ckan/cli/shell.py:33:13 - error: "start_ipython" is not a known attribute of module "IPython" (reportAttributeAccessIssue)
/home/ian/cd/ckan-docker/src/ckan/ckan/lib/helpers.py
  /home/ian/cd/ckan-docker/src/ckan/ckan/lib/helpers.py:1764:34 - error: Expression of type "dict[str, Module("ckan.model")]" is incompatible with declared type "Context"
    "ApiToken" is defined as a ClassVar in protocol
    "Dashboard" is defined as a ClassVar in protocol
    "DomainObject" is defined as a ClassVar in protocol
    "Group" is defined as a ClassVar in protocol
    "Member" is defined as a ClassVar in protocol
    "Package" is defined as a ClassVar in protocol
    "PackageMember" is defined as a ClassVar in protocol
    "PackageRelationship" is defined as a ClassVar in protocol (reportAssignmentType)
/home/ian/cd/ckan-docker/src/ckan/ckan/lib/i18n.py
  /home/ian/cd/ckan-docker/src/ckan/ckan/lib/i18n.py:309:53 - error: Unnecessary "# type: ignore" comment
/home/ian/cd/ckan-docker/src/ckan/ckan/lib/search/index.py
  /home/ian/cd/ckan-docker/src/ckan/ckan/lib/search/index.py:119:23 - error: Argument of type "dict[str, Module("ckan.model") | AlchemySession]" cannot be assigned to parameter "context" of type "Context" in function "plugin_validate"
    "ApiToken" is defined as a ClassVar in protocol
    "Dashboard" is defined as a ClassVar in protocol
    "DomainObject" is defined as a ClassVar in protocol
    "Group" is defined as a ClassVar in protocol
    "Member" is defined as a ClassVar in protocol
    "Package" is defined as a ClassVar in protocol
    "PackageMember" is defined as a ClassVar in protocol
    "PackageRelationship" is defined as a ClassVar in protocol (reportArgumentType)

...
  /home/ian/cd/ckan-docker/src/ckan/ckanext/activity/model/activity.py:780:5 - error: Return type is unknown (reportUnknownParameterType)
  /home/ian/cd/ckan-docker/src/ckan/ckanext/activity/model/activity.py:781:5 - error: Type of parameter "q" is unknown (reportUnknownParameterType)
  /home/ian/cd/ckan-docker/src/ckan/ckanext/activity/model/activity.py:811:5 - error: Return type is unknown (reportUnknownParameterType)
  /home/ian/cd/ckan-docker/src/ckan/ckanext/activity/model/activity.py:812:5 - error: Type of parameter "q" is unknown (reportUnknownParameterType)
/home/ian/cd/ckan-docker/src/ckan/ckanext/tabledesigner/views.py
  /home/ian/cd/ckan-docker/src/ckan/ckanext/tabledesigner/views.py:39:40 - error: Cannot assign to attribute "_prepare" for class "DictionaryView"
    Type "(_i: str, _r: str) -> dict[str, Any]" is incompatible with type "(id: str, resource_id: str) -> dict[str, Any]"
      Parameter name mismatch: "id" versus "_i"
      Parameter name mismatch: "resource_id" versus "_r" (reportAttributeAccessIssue)
93 errors, 0 warnings, 0 informations 

Version:

pyright 1.1.362

@smotornyuk
Copy link
Member

@wardi, the currently used version of pyright is v1.1.339. Most likely you did one of the following:

  • used a global newer version of pyright to check typing: pyright instead of npx pyright
  • installed non-strict dependencies of NPM: npm install(npm i) instead of npm ci

Yes, the newer the better, but it detects more problems and I need to create a separate PR with pyright upgrade. For now try the following:

npm ci
npx pyright

On your branch it gives me only two errors:

/tmp/haha/ckan/ckan/cli/profile.py
  /tmp/haha/ckan/ckan/cli/profile.py:55:34 - error: Cannot access member "getstats" for type "Profile"
    Member "getstats" is unknown (reportGeneralTypeIssues)
  /tmp/haha/ckan/ckan/cli/profile.py:54:34 - error: Cannot access member "getstats" for type "Profile"
    Member "getstats" is unknown (reportGeneralTypeIssues)
2 errors, 0 warnings, 0 informations

For these two errors, add a # type: ignore comment at the end of both lines, I'll try to fix it during pyright upgrade

If you are interested in what caused these 2 errors:

Profile.getstats implemented on C-level. Because of it, pyright cannot automatically detect and use its type definition. Instead, pyright relies on shared type definition.
These definitions are not always complete and in case of cProfile, getstats is missing from the Profile annotations.

To fix the problem we can either make a PR to typeshed, wait for the release and upgrade pyright. It will take too much time so alternatives are using # type: ignore or adding the following lines inside CKAN's cli/profile.py:

    from cProfile import Profile as _Profile
    from typing import Any
    class Profile(_Profile):
        getstats: Any

@smotornyuk
Copy link
Member

smotornyuk commented May 16, 2024

Sorry, just noticed, that I attached a link to the latest version of typeshed with getstats added to annotations 3 months ago. So upgrading pyright will solve the problem and you can safely proceed with the # type: ignore for now

@amercader amercader merged commit f8e9b1f into master May 23, 2024
9 checks passed
@amercader amercader deleted the profile-profile branch May 23, 2024 13:49
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.

None yet

3 participants