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

Issue 337: Added pkg_key to ensure 'packages' == 'dependencies' #359

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

mforbes
Copy link

@mforbes mforbes commented Feb 1, 2022

No description provided.

Michael McNeil Forbes added 11 commits January 29, 2022 13:17
…A_STACK_2.

If the user has `auto_stack: 1` or higher and has a stacked environment
as indicated by `CONDA_STACK_2 == True`, then the PATH environmental
variable might be modified bt prepare.  This change ignores the error if
`CONDA_STACK_2 == True`.  A better resolution would be to control the
configuration of the conda environment before running tests.
This test parameterizes "properties" in all tests in test_project.py
but only tests the "properties".
API change: to_json(pkg_key) now take this argument.
@mforbes mforbes changed the title Issue 337: Added pkg_key to ensure 'packages Issue 337: Added pkg_key to ensure 'packages' == 'dependencies' Feb 1, 2022
@mforbes
Copy link
Author

mforbes commented Feb 1, 2022

The general strategy here is to add an attribute pkg_key to YamlFile, which can then be set as an instance attribute when a file is loaded, specifying that the file contains 'packages' or 'dependencies'. Which one is determined by _get_pkg_key() which looks for one of these in the top level and in env_specs. Currently _get_pkg_key() always returns something: it could in principle check for inconsistencies. This code is in place with assert but these are ignored so as not to muckup tests.

The testing strategy is to introduce a pkg_key fixture passed to tests that had 'packages'. Some of these tests create files from scratch. To test these, I added a decorator @pytest._change_default_pkg_key which will run the test in a context where the default value of YamlFile.pkg_key is changed (then restored). This might skip some cases where a file is inconsistent with the default but should still work (but I added a minimal amount of these, so it is not so likely).

The current implementation breaks python 2.7 compatibility (I use f-strings in many places to keep the new code separate from older code) but this is easily changed with .replace(). (Should we add nox for testing against different versions of python? I suppose that the CI will get this, but it would be nice to be able to test it locally while developing).

The tests which run the redis service do not work yet, and I have not been able to run the slow tests.

@AlbertDeFusco
Copy link
Collaborator

On my Mac I've rebased your branch against #352 where I have fixed the tests, especially the slow ones.

So far only 11 tests fail 👍 🎉 . I'll push a branch you can try for yourself and maybe make a few fixes for these tests.

@AlbertDeFusco
Copy link
Collaborator

Copy link
Collaborator

@AlbertDeFusco AlbertDeFusco left a comment

Choose a reason for hiding this comment

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

Some good stuff here. I'll keep looking at the tests against the soon-to-be-merged PR with corrected tests.

@@ -119,7 +119,7 @@ class ProjectFile(YamlFile):
# Use `anaconda-project add-env-spec` to add environment specs.
#
env_specs: {}
'''
'''.replace('<pkg_key>', YamlFile.pkg_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious about using replace. Is there a reason not to use format? For example,

"The pkg_key is {pkg_key}".format(pkg_key=YamlFile.pkg_key)

Copy link
Author

Choose a reason for hiding this comment

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

There are places (like line 412 in this file) where the original string contains empty dictionaries {}. If we use .format() then we need to also convert all of these to {{}} etc. I started using .format() but then felt it was more explicit if we use a replacement of something that would be unlikely to appear in a .yaml file so that escaping of braces is not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, right. Good call.

yield request.param


def _change_default_pkg_key(test_function):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very cool. So would this only be needed for tests that don't explicitly write packages/dependencies in the file? For example a test that runs init?

Otherwise a test that does supply the list would only need to parameterize on pkg_key to run both scenarios.

Copy link
Author

Choose a reason for hiding this comment

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

No, it should probably be used for some others too that also use the pkg_key fixture to ensure that explicitly using both the default value, and the alternative value works in both cases of default value -- 4 cases. In practice, it is hard for me to imagine a situation where a test works with two of the cases, but fails with the other two, but I can't rule this out, so I provide this in case we stumble on one.

@AlbertDeFusco
Copy link
Collaborator

AlbertDeFusco commented Feb 13, 2022

Another note. There is an easier way to change the pkg_key and you can do it from the parametrized fixture or put the monkeypatch in its own fixture.

in confest.py

import pytest


@pytest.fixture(params=["packages", "dependencies"])
def pkg_key(request, monkeypatch):
    """Ensure equivalence between `dependencies` and `packages`"""
    monkeypatch.setattr('anaconda_project.yaml_file.YamlFile.pkg_key', request.param)
    return request.param

For this case there would likely not be any difference between yield and return. I suppose either one works the same way since there is no other code after the yield.

@AlbertDeFusco AlbertDeFusco mentioned this pull request Feb 14, 2022
12 tasks
@mforbes
Copy link
Author

mforbes commented Feb 14, 2022

Is there a reason to return rather than yield? I took the documentation to be recommending to always yield. (I don't see why there would be a functional difference.)

However, I think that this should be done in a separate fixture default_pkg_key for example, otherwise we will never test against using values of pkg_key that are contrary to the default. The only potential issue with a fixture is that linters might complain that the argument is not used in some tests?

@AlbertDeFusco
Copy link
Collaborator

AlbertDeFusco commented Feb 15, 2022

Having them separate and using yield in pkg_key works for me. I would recommend using the oneline monkeypatch directly in the tests that need it rather than as a fixture or decorator. For example, this test would not change the default

def test_add_command_specifying_notebook(monkeypatch, capsys, pkg_key):
    def check_specifying_notebook(dirname):
        args = Args('notebook', 'test', 'file.ipynb', directory=dirname)
        res = main(args)
        assert res == 0

        project = Project(dirname)

        command = project.project_file.get_value(['commands', 'test'])
        assert command['notebook'] == 'file.ipynb'
        assert command['env_spec'] == 'default'
        assert len(command.keys()) == 2

    with_directory_contents_completing_project_file({DEFAULT_PROJECT_FILENAME: f'{pkg_key}:\n - notebook\n'},
                                                    check_specifying_notebook)

And a test that does the monkeypatch would have this extra line

def test_add_command_generates_env_spec_suggestion(pkg_key):
    monkeypatch.setattr('anaconda_project.yaml_file.YamlFile.pkg_key', pkg_key)

    def check_add_command(dirname):
        project = project_no_dedicated_env(dirname)
        assert project.problems == []
...

@mforbes
Copy link
Author

mforbes commented Feb 15, 2022

I think it might be better to keep both fixtures so it is clear which tests are testing different default values and which are testing different explicit values. I will try to take a stab at this today. There are some cases where I need input:

What should happen if a user uses both packages and dependences in the same file? Is this allowed, or should this be treated as an error? (I think the current code will try to preserve this, but need to test this more clearly.)

Another example istest_project.py::test_no_auto_fix_env_spec_with_notebook_bokeh_injection which uses get_value() and set_value(). Everything has to be matched carefully to work right now, but I could imagine that internally we might want to treat these as equal.

My preference would be to migrate everything towards dependencies for conda compatibility, and not make any special cases except backwards compatibility with files that have packages everywhere, but I have not seen a discussion about whether we should try to support mixed version.

@AlbertDeFusco
Copy link
Collaborator

I'd like to have dependencies as default. A user may have Yaml aliased dependencies to packages, so a stern warning might be useful.

@mforbes
Copy link
Author

mforbes commented Feb 16, 2022

Okay, then I suggest reworking things to simply convert packages to dependencies on input/read, and then always output dependences. That will keep things simpler.

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