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

Implement typings in conda-smithy #1

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

Conversation

ninetteadhikari
Copy link
Member

@ninetteadhikari ninetteadhikari commented May 16, 2024

Introduction

This work is done according to "Milestone 2: Implement typings in conda-smithy" as stated in the Scope of Work with Sovereign Tech Fund (STF) (https://www.sovereigntechfund.de/).

This PR addresses the typing request from this issue

Setup

Setup details are here: https://github.com/neighbourhoodie/conda-smithy/wiki

Steps followed to add type

As suggested in the ticket in the conda-smithy repo, MonkeyType is used to generate type hints for the python code.

Generate type hints

  1. Install MonkeyType
pip install MonkeyType
  1. Run tests using MonkeyType. This will dump call traces into a SQLite database in the file monkeytype.sqlite3 in the current working directory.
monkeytype run -m pytest
  1. Make a separate folder called stubs to save the type hint files
mkdir stubs
  1. Generate stub files for each of the modules found in the SQLite database
monkeytype list-modules | xargs -n1 -I{} sh -c 'monkeytype stub {} > stubs/{}.pyi'

The above command will create stub files in the stubs folder. Here's an example of what the content of a stub file will look like. It gives suggestions for types.

def rotate_anaconda_token(
    user: str,
    project: str,
    feedstock_config_path: Optional[str],
    drone: bool = ...,
    circle: bool = ...,
    travis: bool = ...,
    azure: bool = ...,
    appveyor: bool = ...,
    github_actions: bool = ...,
    token_name: str = ...,
    drone_endpoints: List[str] = ...
): ...
  1. The suggestions generated in the stub file were used make changes to the actual python code.

Checklist

  • Ran monkeytypes to get type suggestions
  • Add type to conda_smithy modules
  • Add type to tests
  • Add documentation on using Monkeytype
  • Add mypy for type checks

@coveralls
Copy link

coveralls commented Jun 7, 2024

Pull Request Test Coverage Report for Build 9419614194

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 130 of 131 (99.24%) changed or added relevant lines in 11 files are covered.
  • 16 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 67.943%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/variant_algebra.py 6 7 85.71%
Files with Coverage Reduction New Missed Lines %
conda_smithy/lint_recipe.py 16 94.24%
Totals Coverage Status
Change from base Build 9299597764: 0.1%
Covered Lines: 2807
Relevant Lines: 4049

💛 - Coveralls

@@ -66,6 +66,10 @@ jobs:
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}

- name: run type check
run: |
mypy conda_smithy tests
Copy link

Choose a reason for hiding this comment

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

We only check the types at tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

i added type check for the folders 'conda_smithy' and 'tests'. wasn't sure if other files/folders require type check to run as well..

appveyor: bool = True,
github_actions: bool = True,
token_name: str = "BINSTAR_TOKEN",
drone_endpoints = (),
Copy link

Choose a reason for hiding this comment

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

Question: this is a type that doesn't exist? can't we at least use list or function? (what is this?)

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems like list | tuple worked. Only adding list was throwing error that () is not a list

Copy link

@jcoglan jcoglan Jun 13, 2024

Choose a reason for hiding this comment

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

My best guess is that () is the empty tuple, which is not a very useful value to pass to any function.

subcommand = None
aliases = []
subcommand: Optional[str] = None
aliases: list = []
Copy link

Choose a reason for hiding this comment

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

Do we know the type of the list here so it can be List?

Copy link
Member Author

Choose a reason for hiding this comment

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

added List[str]

@@ -208,7 +260,7 @@ def break_up_top_level_values(top_level_keys, squished_variants):
zip_key_groups = squished_variants["zip_keys"]
if zip_key_groups and not isinstance(zip_key_groups[0], list):
zip_key_groups = [zip_key_groups]
zipped_configs = []
zipped_configs: list = []
top_level_dimensions = []
Copy link

Choose a reason for hiding this comment

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

what about the top_level_dimensions?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops i missed that! added.

for idx, variant_key in enumerate(squished_variants[key]):
top_level_config = []
top_level_config: Any = []
Copy link

Choose a reason for hiding this comment

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

I am sure you checked this, but can we avoid Any?

Copy link
Member Author

@ninetteadhikari ninetteadhikari Jun 10, 2024

Choose a reason for hiding this comment

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

this is a tricky one:( top_level_config is being declared as a list initially and then being converted to tuple on line 287. i tried using list | tuple but then it throws another error for append. If I leave out the type altogether this append error still persists:(
Item "tuple[Any, ...]" of "list[Any] | tuple[Any, ...]" has no attribute "append"
I added Any to avoid the errors since i couldn't find any other appropriate type 🙈

Copy link

Choose a reason for hiding this comment

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

If something is being converted to a different type, you could try using a new variable name when the conversion happens and giving each variable a distinct type.

Comment on lines 605 to 575
top_level_loop_vars = set()

all_used_vars = set()
all_variants = set()
all_variants: Any = set()

Copy link

Choose a reason for hiding this comment

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

top_level_loop_vars and all_used_vars are already set?

Copy link
Member Author

Choose a reason for hiding this comment

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

for some reason mypy was complaining that all_variants needs a type. So I added Any but now I updated it and added type as set. Also updated the function _merge_deployment_target on line 506 to set for the types to align.

check=False,
temporary_directory=None,
forge_file_directory: str,
forge_yml: None = None,
Copy link

Choose a reason for hiding this comment

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

Is this always none?

Copy link
Member Author

Choose a reason for hiding this comment

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

oops no, i updated it to be Optional[str] to have both str and None. (I use Optional instead of | because | is causing error for python 3.8.19, which is what github actions is running..)



def get_repo(path, search_parent_directories=True):
from io import TextIOWrapper
Copy link

Choose a reason for hiding this comment

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

Please check this import

Copy link

Choose a reason for hiding this comment

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

now I saw it. Mh... I am unsure

Copy link
Member Author

Choose a reason for hiding this comment

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

this was a suggestion when I ran monkeytype, but if it doesn't make sense we can take it out..

@@ -95,7 +95,7 @@ class BotConfigVersionUpdatesSourcesChoice(StrEnum):
class AzureRunnerSettings(BaseModel):
"""This is the settings for runners."""

model_config: ConfigDict = ConfigDict(extra="allow")
model_config = ConfigDict(extra="allow")
Copy link

Choose a reason for hiding this comment

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

why did you remove the type?

Copy link
Member Author

Choose a reason for hiding this comment

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

this error keeps showing up for ConfigDict. wasn't sure how to address this 🙈
Cannot override class variable (previously declared on base class "BaseModel") with instance variable

@@ -11,17 +11,23 @@

import git
from git.index.typ import BlobFilter
from git.repo.base import Repo
Copy link

Choose a reason for hiding this comment

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

do we need this import?

Copy link
Member Author

Choose a reason for hiding this comment

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

removed it.

@@ -5,6 +5,9 @@

import pytest
import scrypt
from _pytest._py.path import LocalPath
Copy link

Choose a reason for hiding this comment

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

Question: What is the difference between pytest and _pytest?

OrderedDict,
List[Union[str, List[str]]],
List[List[str]],
],
Copy link

Choose a reason for hiding this comment

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

I am not familiar with mypy specifically, so this is a hunch based on other type systems: I would expect the type List[Union[str, List[str]]] to be identical to Union[List[str], List[List[str]]], so can this whole type be reduced to Union[List[Union[str, List[str]]], OrderedDict]?

Copy link

Choose a reason for hiding this comment

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

Or, if the nested unions are too confusing, Union[List[str], List[List[str]], OrderedDict]?

@@ -107,7 +136,7 @@ def package_key(config, used_loop_vars, subdir):
return key.replace("*", "_").replace(" ", "_")


def _ignore_match(ignore, rel):
def _ignore_match(ignore: Union[Set[str], Tuple[()]], rel: str):
Copy link

Choose a reason for hiding this comment

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

Did we find out what the type () refers to? I can't find it in the mypy docs, and Tuple[()] (i.e. a tuple with a single element of type ()) doesn't seem very useful.

Copy link

Choose a reason for hiding this comment

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

Ah ok, Tuple[()] is the type for the empty tuple; if you use () as a type mypy will tell you to use Tuple[()] or None instead.

return sorted(range(len(seq)), key=seq.__getitem__)


def sort_config(config, zip_key_groups):
def sort_config(
config: OrderedDict, zip_key_groups: List[Union[List[str], Any]]
Copy link

Choose a reason for hiding this comment

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

Is the type Union[T, Any] equivalent to Any? If not, what makes it different and how are you allowed to use the value that might be a T or anything else?

Copy link
Member Author

Choose a reason for hiding this comment

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

adding the discussion from slack here

https://neighbourhoodie.slack.com/archives/C069J3L39HN/p1718277724884569?thread_ts=1717776546.052819&cid=C069J3L39HN
if x: Union[A, B, C...], then for the call x.foo(), all types A, B, C etc need to have a foo() method
so that means Union[T, Any] and Any are not the same thing; it means a value could be anything but you need to wrap it in isinstance(val, T) before using it
and then you get the benefits of type-checking T, whereas if you'd just used Any you'd get no type checks at all

OrderedDict,
List[Union[str, List[str]]],
List[List[str]],
],
Copy link

Choose a reason for hiding this comment

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

See earlier comment about reducing the complexity of this type.

Tuple[str, str, str, str],
Tuple[str],
Tuple[str, str, str, str, str],
List[List[str]],
Copy link

Choose a reason for hiding this comment

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

These tuples look odd -- this really looks like the function really expects an arbitrary-size collection of strings (List[str], Set[str] etc) but some callers are passing this as a tuple. Rather than specifying every size of tuple we accept I would treat this as a signal to go and fix the callers to stop them using tuples.

Copy link
Member Author

Choose a reason for hiding this comment

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

i changed it to this Union[set, dict, list, tuple] keeping it generic.. not sure if this helps to reduce complexity..

List[Union[str, List[str]]],
List[List[str]],
],
],
Copy link

Choose a reason for hiding this comment

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

c.f. earlier comment to simplify this type.

Copy link
Member Author

@ninetteadhikari ninetteadhikari Jun 18, 2024

Choose a reason for hiding this comment

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

simplified it to generic Union[list, OrderedDict]

str,
Union[
List[str], OrderedDict, List[Union[str, List[str]]], List[List[str]]
],
Copy link

Choose a reason for hiding this comment

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

Again this is equivalent to Union[List[str], List[List[str]], OrderedDict].

Union[List[str], Dict[str, Dict[str, str]], List[List[str]]],
],
]
] = None,
Copy link

Choose a reason for hiding this comment

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

I think this whole type simplifies to Optional[Dict[str, Union[List[str], List[List[str]], Dict[str, Dict[str, str]]]]].

],
],
Dict[str, Union[str, bool]],
],
Copy link

Choose a reason for hiding this comment

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

I believe this type is equivalent to:

Union[
    CommentedMap,
    Dict[str, bool],
    Dict[str, str],
    Dict[str, List[str]],
    Dict[str, Dict[str, bool]],
    Dict[str, Dict[str, Dict[str, str]]]
]

Or, if you want to remove a bit of duplication without too much extra union nesting:

Union[
    CommentedMap,
    Dict[str, Union[
        bool,
        str,
        List[str],
        Dict[str, bool],
        Dict[str, Dict[str, str]]
    ]]
]

I think either of these is more readable than the existing type.

@coveralls
Copy link

coveralls commented Jun 19, 2024

Pull Request Test Coverage Report for Build 9587371809

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 144 of 146 (98.63%) changed or added relevant lines in 11 files are covered.
  • 23 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 67.996%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Files with Coverage Reduction New Missed Lines %
conda_smithy/lint_recipe.py 23 94.31%
Totals Coverage Status
Change from base Build 9299597764: 0.2%
Covered Lines: 2809
Relevant Lines: 4051

💛 - Coveralls

@@ -14,12 +14,12 @@

_thisdir = os.path.abspath(os.path.dirname(__file__))

InitArgs = collections.namedtuple(
InitArgs = collections.namedtuple( # type: ignore
"ArgsObject",
Copy link

Choose a reason for hiding this comment

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

I think you can resolve this by passing "InitArgs" as the first argument instead of "ArgsObject", if I understood this correctly. Makes no difference to the tests, at least.

Same for the one below, pass in "RegenerateArgs" instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

i was afraid this might break the test so i ignored it but it worked! updated it.

Copy link

@espy espy left a comment

Choose a reason for hiding this comment

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

Sorry, I meant to just make comments, not start a review 🙈

@@ -12,7 +12,7 @@
from conda.base.constants import KNOWN_SUBDIRS

try:
from enum import StrEnum
from enum import StrEnum # type: ignore
except ImportError:
from backports.strenum import StrEnum
Copy link

Choose a reason for hiding this comment

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

I think it's better to ignore the error on the second import, at least that’s what I gather from here.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm moving the ignore to the second import StrEnum brings back the error: Module "enum" has no attribute "StrEnum" :(

@coveralls
Copy link

coveralls commented Jun 20, 2024

Pull Request Test Coverage Report for Build 9596910568

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 144 of 146 (98.63%) changed or added relevant lines in 11 files are covered.
  • 23 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 67.996%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Files with Coverage Reduction New Missed Lines %
conda_smithy/lint_recipe.py 23 94.31%
Totals Coverage Status
Change from base Build 9299597764: 0.2%
Covered Lines: 2809
Relevant Lines: 4051

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 20, 2024

Pull Request Test Coverage Report for Build 9599544458

Details

  • 146 of 148 (98.65%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 67.996%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Totals Coverage Status
Change from base Build 9563050021: 0.09%
Covered Lines: 2809
Relevant Lines: 4051

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 20, 2024

Pull Request Test Coverage Report for Build 9599610108

Details

  • 146 of 148 (98.65%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 67.996%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Totals Coverage Status
Change from base Build 9563050021: 0.09%
Covered Lines: 2809
Relevant Lines: 4051

💛 - Coveralls

@ninetteadhikari ninetteadhikari changed the title add type for conda_smithy Implement typings in conda-smithy Jun 20, 2024
@coveralls
Copy link

coveralls commented Jun 20, 2024

Pull Request Test Coverage Report for Build 9599947730

Details

  • 143 of 145 (98.62%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 67.996%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Totals Coverage Status
Change from base Build 9563050021: 0.09%
Covered Lines: 2809
Relevant Lines: 4051

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 27, 2024

Pull Request Test Coverage Report for Build 9702412366

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 143 of 145 (98.62%) changed or added relevant lines in 11 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.06%) to 67.963%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Files with Coverage Reduction New Missed Lines %
conda_smithy/configure_feedstock.py 2 82.98%
Totals Coverage Status
Change from base Build 9563050021: 0.06%
Covered Lines: 2808
Relevant Lines: 4051

💛 - Coveralls

@ninetteadhikari ninetteadhikari force-pushed the m2-add-type branch 2 times, most recently from 82d1fb1 to b2a66d4 Compare June 27, 2024 19:29
@coveralls
Copy link

coveralls commented Jun 27, 2024

Pull Request Test Coverage Report for Build 9702463236

Details

  • 143 of 145 (98.62%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 67.963%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Totals Coverage Status
Change from base Build 9702449954: 0.09%
Covered Lines: 2808
Relevant Lines: 4051

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 27, 2024

Pull Request Test Coverage Report for Build 9702496514

Details

  • 143 of 145 (98.62%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.09%) to 67.963%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Totals Coverage Status
Change from base Build 9702449954: 0.09%
Covered Lines: 2808
Relevant Lines: 4051

💛 - Coveralls

@ninetteadhikari ninetteadhikari force-pushed the m2-add-type branch 2 times, most recently from 00b9c49 to a4e89f7 Compare August 23, 2024 10:45
@coveralls
Copy link

coveralls commented Aug 28, 2024

Pull Request Test Coverage Report for Build 10599090712

Details

  • 131 of 133 (98.5%) changed or added relevant lines in 14 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 70.283%

Changes Missing Coverage Covered Lines Changed/Added Lines %
conda_smithy/configure_feedstock.py 59 60 98.33%
conda_smithy/variant_algebra.py 6 7 85.71%
Totals Coverage Status
Change from base Build 10598408429: 0.06%
Covered Lines: 3189
Relevant Lines: 4458

💛 - Coveralls

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