this is great, but it's getting quite complicated 😄 how about taking this part out to its own function (which accepts yaml_file as an argument), and then unit-testing that?

Sounds good! I've separated this portion of code into its own function. I'm writing unit tests now.

thanks!

BTW if you notice some CI failures for some unrelated jobs, that's probably exactly what they are (i.e. unrelated)

I wrote a new unit test file at scripts/tests/test_validate_min_versions_in_sync.py.

It comes with 3 additional files under scripts/tests/:

- dummy_toml_file.toml
- dummy_yaml_expected_file_1.yaml
- dummy_yaml_unmodified_file_1.yaml

The .toml file can be adjusted to change the baselines for the minimum dependencies. The dummy_yaml_unmodified_file_1.yaml can be adjusted to test different edge cases, however, the dummy_yaml_expected_file_1.yaml should be changed accordingly to reflect the proper results for the previously mentioned file, since the two should be in sync as a testing set.

I was thinking of creating more sets of expected/unmodified YAML files so that each set can test its own edge case like duplicates, same version, no version, range, etc. That way, each unit test can be smaller and more meaningful.

Currently, I think the dataset is too large to provide meaningful test assertion results.

@MarcoGorelli What are your thoughts? Feedback appreciated!

nice! this isn't user-facing, so arguably doesn't need too extensive testing. but yes, smaller test files would make it easier to reason about what the script is doing

pylint is suggesting to rewrite this one

 pylint...................................................................Failed
- hook id: pylint
- duration: 541.86s
- exit code: 8

************* Module scripts.validate_min_versions_in_sync
scripts/validate_min_versions_in_sync.py:93:15: R1714: Consider merging these comparisons with 'in' by using 'operator in ('<', '>')'. Use a set instead if elements are hashable. (consider-using-in)

(pylint is very slow, so it currently only runs in CI - but ruff is re-implementing a large portion of its checks, hopefully we can remove it and just use ruff soon-ish, which is blazingly fast)

is this to find the unique dependencies? perhaps using a set would be clearer, like

res = list({x for x in yaml_dependencies})

Yes. I was using a set before but when I would run pre-commit, I get this:
TypeError: unhashable type: 'dict'

do you remember which hook was throwing that? pretty sure this is valid syntax

In [15]: yaml_dependencies = [1,2,3, 3]

In [16]: list({x for x in yaml_dependencies})
Out[16]: [1, 2, 3]

thanks, I'll take a look

I think it is because of
tandard>=0.15.2', 'dask-core', 'seaborn-base', 'moto', 'flask', 'asv>=0.5.1', 'c-compiler', 'cxx-compiler', 'black=23.1.0', 'cpplint', 'flake8=6.0.0', 'isort>=5.2.1', 'mypy=1.0', 'pre-commit>=2.15.0', 'pyupgrade', 'ruff=0.0.215', 'gitpython', 'gitdb', 'natsort', 'numpydoc', 'pydata-sphinx-theme<0.11', 'pytest-cython', 'sphinx', 'sphinx-panels', 'sphinx-copybutton', 'types-python-dateutil', 'types-PyMySQL', 'types-pytz', 'types-setuptools', 'nbconvert>=6.4.5', 'nbsphinx', 'pandoc', 'ipywidgets', 'nbformat', 'notebook>=6.0.3', 'ipykernel', 'jinja2>=3.0.0', 'markdown', 'feedparser', 'pyyaml', 'requests', {'pip': ['sphinx-toggleprompt']}]

In one of the yaml_dependencies, there is {'pip': ['sphinx-toggleprompt']}]

That's also why I added this portion:

if isinstance(dependency, dict) or dependency in EXCLUSION_LIST: continue

To ignore that nested dictionary

ah, thanks for explaining!

Would it work to apply

$ git diff
diff --git a/scripts/validate_min_versions_in_sync.py b/scripts/validate_min_versions_in_sync.py
index b1dc734994..6e3c93dd57 100755
--- a/scripts/validate_min_versions_in_sync.py
+++ b/scripts/validate_min_versions_in_sync.py
@@ -62,9 +62,6 @@ def pin_min_versions_to_ci_deps():
             yaml_file_data = yaml_f.read()
             yaml_file = yaml.safe_load(yaml_file_data)
             yaml_dependencies = yaml_file["dependencies"]
-            res = []
-            [res.append(x) for x in yaml_dependencies if x not in res]
-            yaml_dependencies = res
             yaml_map = get_yaml_map_from(yaml_dependencies)
             toml_map = get_toml_map_from(toml_dependencies)
             yaml_file_data = pin_min_versions_to_yaml_file(
@@ -105,6 +102,8 @@ def get_yaml_map_from(yaml_dic) -> dict[str, str]:
     for dependency in yaml_dic:
         if type(dependency) == dict or dependency in EXCLUSION_LIST:
             continue
+        if dependency in yaml_map:
+            continue
         search_text = str(dependency)
         operator = get_operator_from(search_text)
         if "," in dependency:

?

I would help if you could also add a short little docstring to pin_min_versions_to_ci_deps in which you note that pip dependencies won't be pinned (which is definitely OK for now, what you've done already adds value)

Nice, it works! I added the docstring, along with all the other requested updates.

I don't think this is necessary (the next line will overwrite the file)

I think

if isinstance(dependency, dict)

would be more idiomatic

same comment as above - can we create this in one go instead of first creating an empty list and then appending to it within a list comprehension?

psutil was recently removed from all of the these files. Could you remove them?

Instead of these comments could you put this comment in the file name like random_deps.toml

Also for the directory naming could you use something like scripts/tests/data/?

wait this undoes the pylint nit 😆

 ************* Module scripts.validate_min_versions_in_sync
scripts/validate_min_versions_in_sync.py:132:4: C0200: Consider using enumerate instead of iterating with range and len (consider-using-enumerate)

maybe we can just remove this one

Since this runs on every commit, we don't necessarily want to rewrite this file every time.