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 4371 incorrect dependencies when install dev packages #5234

Conversation

dqkqd
Copy link
Contributor

@dqkqd dqkqd commented Aug 8, 2022

The issue

I'm trying to fix #4371. I think it's better to consider 2 cases below:

  • Installing from Pipfile.
  • Installing from provided packages (the issue itself).

The fix

Installing from Pipfile

I read through the code, and here is what it does when installing packages from Pipfile:

  1. Resolve dev-packages dependencies. Write to lockfile as default.
  2. Resolve packages dependencies. Write to lockfile as develop.
  3. Overwrite duplicated develop packages by default packages.
  4. Install default and develop packages in the lockfile.

Because, dev-packages is resolved separately, there might be a chance where dev-packages is overwritten but its dependencies are not (they aren't duplicated with packages). So I think the result dependency graph would be more accurate if packages is considered as constraint while resolving dev-packages dependencies. Luckily pip provided constraints files.

I modified the code to do the followings:

  1. Resolve packages dependencies. Write to lockfile.
  2. Create constraints file from packages. Skip editable and extras packages as describe in pip constraints files.
  3. Resolve dev-packages dependencies using constraints file above. Write to lockfile as default.
  4. Overwrite duplicated packages. Although dev-packages was resolved with constraints, this step is also important because there some cases constraints might be skipped (editable and extras packages).
  5. Install resolved packages.

Installing from provided packages

When packages are passed as arguments, pipenv does the followings:

  1. Install passed packages without resolving dependencies. Write to Pipfile and lockfile.
  2. Resolving dev-packages and packages independently. Modify the lockfile.
  3. Overwrite duplicated packages.
  4. Install all the packages in the lockfile again. There might be a case it will uninstall the packages from step 1 before installing.

Similar to the first problem, I tried to use default packages as constraints to resolve packages dependencies. I think it is enough to just resolve dev-packages at step 1. Because default packages should be installed directly, and resolved packages at step 2 don't need to be resolved again.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix, .feature, .behavior, .doc. .vendor. or .trivial (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

pipenv/core.py Outdated
project,
requirements_dir=requirements_dir,
)
pip_command.extend(["-c", vistir.path.normalize_path(constraint_filename)])
Copy link
Member

Choose a reason for hiding this comment

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

Is there an alternative function that would make sense other than calling something from vistir? Also, is there a way to do this without writing to a file?

Copy link
Contributor Author

@dqkqd dqkqd Aug 9, 2022

Choose a reason for hiding this comment

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

Thanks for reviewing.

There are similar function to normalize path inside shell.py, I will replace vistir with this.

def normalize_path(path):
return os.path.expandvars(
os.path.expanduser(os.path.normcase(os.path.normpath(os.path.abspath(str(path)))))
)

After reading constraints files, I think currently there are no way to pass constraint as argument to pip.

  • The constraint file created from write_to_constrain_file is passed to subprocess, so it might be hard to not use the file.

    pipenv/pipenv/core.py

    Lines 1609 to 1610 in 30a6b1a

    c = subprocess_run(pip_command, block=block, env=pip_config)
    c.env = pip_config

  • The constraints files create from default_constraint_file inside resolver could be removed I think. But this might take time reading pip code base to see how constraints files is convert into constraint line.

pipenv/core.py Outdated
for k, v in project.packages.items()
if not is_editable(k) and not is_editable(v) and not has_extras(v)
}
constraints = convert_deps_to_pip(constraints, project=project, r=False)
Copy link
Member

Choose a reason for hiding this comment

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

convert_deps_to_pip calls quite a bit of code in requirementslib. I've been studying this code base for 10 months now and I still haven't wrapped my head around all of the resolver/dependency logic, but from spending some time in the debugger last night I know that this method calls a lot of code that we need help unraveling in requirementslib. I am concerned that this PR may be adding to the amount of logic we have to understand around this, and could possibly be duplicating some code that we should be refactoring elsewhere. I'll definitely need more time to study this change, but I am still glad you are pushing forward with a solution to the original issue.

Copy link
Contributor Author

@dqkqd dqkqd Aug 9, 2022

Choose a reason for hiding this comment

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

Sorry, the duplicated code I mentioned is actually myself.

pipenv/pipenv/core.py

Lines 1462 to 1470 in 30a6b1a

# duplicated in pipenv.utils.resolver
# https://pip.pypa.io/en/stable/user_guide/#constraints-files
# constraints must have a name, they cannot be editable, and they cannot specify extras.
constraints = {
k: v
for k, v in project.packages.items()
if not is_editable(k) and not is_editable(v) and not has_extras(v)
}
constraints = convert_deps_to_pip(constraints, project=project, r=False)

def get_default_constraints_from_packages():
# https://pip.pypa.io/en/stable/user_guide/#constraints-files
# constraints must have a name, they cannot be editable, and they cannot specify extras.
constraints = {
k: v
for k, v in self.project.packages.items()
if not is_editable(k) and not is_editable(v) and not has_extras(v)
}
constraints = convert_deps_to_pip(constraints, project=self.project, r=False)
return constraints

I'm not sure should I write an attribute which generate constraints packages inside class Project or not.

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest extracting it from resolver.py to a standalone util, that doesn't depend on a class and re-use it in both places. Instead of self.project it can just take a project as a parameter. I am not sure if it makes more sense to live in "resolver.py" or "dependencies.py" but dependencies.py already has the methods convert_deps_to_pip and is_editable so I am leaning towards putting it there, plus its a smaller file than resolver.py which is quite large still--would be good to chip away at the code organization improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, it seems reasonable to have it inside "dependencies.py". I updated the code.

@@ -791,3 +791,39 @@ def test_pipenv_respects_package_index_restrictions(PipenvInstance):
'sha256:ec22d826a36ed72a7358ff3fe56cbd4ba69dd7a6718ffd450ff0e9df7a47ce6a'],
'index': 'local', 'version': '==2.19.1'}
assert p.lockfile['default']['requests'] == expected_result


@flaky
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be adding any new flaky tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove it. Frankly, I don't know what it does, I use it because I saw other tests functions use it as well.

Copy link
Member

Choose a reason for hiding this comment

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

I think it was considered for tests that would sometimes fail to have them re-run, but we expect our tests to pass so its just an odd annotation.

@pytest.mark.dev
@pytest.mark.lock
@pytest.mark.install
def test_dev_lock_use_default_packages_as_constraint(PipenvInstance):
Copy link
Member

Choose a reason for hiding this comment

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

When I run this test in the debugger, it gets to the method pip_install but use_constraint=False and so it does not exercise the method write_constraint_to_file as I would have thought. Going to try with your other tests to see.

Copy link
Contributor Author

@dqkqd dqkqd Aug 9, 2022

Choose a reason for hiding this comment

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

Actually there are 2 constraints files. The first one is inside resolver.py

@property
def default_constraint_file(self):
if self._default_constraint_file is None:
self._default_constraint_file = self.prepare_default_constraint_file()
return self._default_constraint_file

The second one is write_constraint_to_file as mention above.

  • The first constraints file is created and used when: Locking or Installing with Pipfile using --dev flag (user don't pass any packages args).
  • The second constrains file is created and used when: User pass packages as argument using --dev flag.

When locking, the code does the following things:

  • Resolve dependencies with venv_resolve_deps (this create and use the first constraints file).
  • Running batch_install, install all packages in the lock file. This one calls pip_install, this could create the second constraints file (if both dev and use_constraint equal True). But since all the packages were resolved in step 1 and written to lockfile, so resolving isn't necessary.

    pipenv/pipenv/core.py

    Lines 785 to 793 in 30a6b1a

    no_deps=skip_dependencies,
    block=is_blocking,
    index=dep.index,
    requirements_dir=requirements_dir,
    pypi_mirror=pypi_mirror,
    trusted_hosts=trusted_hosts,
    extra_indexes=extra_indexes,
    use_pep517=use_pep517,
    use_constraint=False, # no need to use constraints, it's written in lockfile

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @dqkqd -- I was able to hit that breakpoint in the test test_install_dev_use_default_constraints after I posted the first message. Can you explain more why to have two different methods of similar code for generating the two different constraints files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have lock and install method. The first constraint file in resolver.py is used for lock, the second one in core.py is used for install.

  1. The first one should not be removed, because it would cause lockfile is not generated correctly.
  2. The second one could be remove. Actually, this is my first approach.
    Suppose I remove this one. Then pip_install inside this loop should not be called, because it will install packages without resolving dependencies.

    pipenv/pipenv/core.py

    Lines 2136 to 2271 in 30a6b1a

    for pkg_line in pkg_list:
    click.secho(
    fix_utf8(f"Installing {pkg_line}..."),
    fg="green",
    bold=True,
    )
    # pip install:
    with vistir.contextmanagers.temp_environ(), create_spinner(
    "Installing...", project.s
    ) as sp:
    if not system:
    os.environ["PIP_USER"] = "0"
    if "PYTHONHOME" in os.environ:
    del os.environ["PYTHONHOME"]
    sp.text = f"Resolving {pkg_line}..."
    try:
    pkg_requirement = Requirement.from_line(pkg_line)
    except ValueError as e:
    sp.write_err("{}: {}".format(click.style("WARNING", fg="red"), e))
    sp.red.fail(
    environments.PIPENV_SPINNER_FAIL_TEXT.format(
    "Installation Failed"
    )
    )
    sys.exit(1)
    sp.text = "Installing..."
    try:
    sp.text = f"Installing {pkg_requirement.name}..."
    if project.s.is_verbose():
    sp.hide_and_write(
    f"Installing package: {pkg_requirement.as_line(include_hashes=False)}"
    )
    c = pip_install(
    project,
    pkg_requirement,
    ignore_hashes=True,
    allow_global=system,
    selective_upgrade=selective_upgrade,
    no_deps=False,
    pre=pre,
    dev=dev,
    requirements_dir=requirements_directory,
    index=index_url,
    pypi_mirror=pypi_mirror,
    use_constraint=True,
    )
    if c.returncode:
    sp.write_err(
    "{} An error occurred while installing {}!".format(
    click.style("Error: ", fg="red", bold=True),
    click.style(pkg_line, fg="green"),
    ),
    )
    sp.write_err(f"Error text: {c.stdout}")
    sp.write_err(click.style(format_pip_error(c.stderr), fg="cyan"))
    if project.s.is_verbose():
    sp.write_err(
    click.style(format_pip_output(c.stdout), fg="cyan")
    )
    if "setup.py egg_info" in c.stderr:
    sp.write_err(
    "This is likely caused by a bug in {}. "
    "Report this to its maintainers.".format(
    click.style(pkg_requirement.name, fg="green")
    )
    )
    sp.red.fail(
    environments.PIPENV_SPINNER_FAIL_TEXT.format(
    "Installation Failed"
    )
    )
    sys.exit(1)
    except (ValueError, RuntimeError) as e:
    sp.write_err("{}: {}".format(click.style("WARNING", fg="red"), e))
    sp.red.fail(
    environments.PIPENV_SPINNER_FAIL_TEXT.format(
    "Installation Failed",
    )
    )
    sys.exit(1)
    # Warn if --editable wasn't passed.
    if (
    pkg_requirement.is_vcs
    and not pkg_requirement.editable
    and not project.s.PIPENV_RESOLVE_VCS
    ):
    sp.write_err(
    "{}: You installed a VCS dependency in non-editable mode. "
    "This will work fine, but sub-dependencies will not be resolved by {}."
    "\n To enable this sub-dependency functionality, specify that this dependency is editable."
    "".format(
    click.style("Warning", fg="red", bold=True),
    click.style("$ pipenv lock", fg="yellow"),
    )
    )
    sp.write(
    "{} {} {} {}{}".format(
    click.style("Adding", bold=True),
    click.style(f"{pkg_requirement.name}", fg="green", bold=True),
    click.style("to Pipfile's", bold=True),
    click.style(
    "[dev-packages]" if dev else "[packages]",
    fg="yellow",
    bold=True,
    ),
    click.style(fix_utf8("..."), bold=True),
    )
    )
    # Add the package to the Pipfile.
    if index_url:
    index_name = project.add_index_to_pipfile(
    index_url, verify_ssl=index_url.startswith("https:")
    )
    pkg_requirement.index = index_name
    try:
    project.add_package_to_pipfile(pkg_requirement, dev)
    except ValueError:
    import traceback
    sp.write_err(
    "{} {}".format(
    click.style("Error:", fg="red", bold=True),
    traceback.format_exc(),
    )
    )
    sp.fail(
    environments.PIPENV_SPINNER_FAIL_TEXT.format(
    "Failed adding package to Pipfile"
    )
    )
    sp.ok(
    environments.PIPENV_SPINNER_OK_TEXT.format("Installation Succeeded")
    )
    # Update project settings with pre preference.
    if pre:
    project.update_settings({"allow_prereleases": pre})

    So pip_install should be replaced by another check_installable_package method. After that, resolving all the packages with do_lock (which use the first constraints file) will give us the same result.
    I didn't use this approach because:
    • $ pip search is not working, I could not find a way to check if a package is installable without calling $ pip install
    • The loop locks scary.

Copy link
Contributor Author

@dqkqd dqkqd Aug 9, 2022

Choose a reason for hiding this comment

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

Also, there is another case where resolving dependencies inside pip_install is necessary.
For example, I have packages installed with this Pipfile:

[packages]
django = "<3.0"

Suppose I install new dev package using command $ pipenv install --dev "django>3.0,<4.0". Clearly the package is installable but it shouldn't be, because it would conflict with default packages.

default_constraints_file.close()
return default_constraints_file.name

@property
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a @cached_property instead so that we don't have to implement that ourselves with if self._default_constraint_file is None: logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used @cached_property but falling some tests on MacOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those tests also failed without @cached_property as well, so maybe @cached_property is not the issue. Because @cached_property does not work with python 3.7, I used @property and @lru_cache instead.

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem is you need to import @cached_property from the vendor'd dependencies, go with from pipenv.vendor.cached_property import cached_property until #5169 is completed.

Copy link
Member

Choose a reason for hiding this comment

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

The only tests that are expected could fail, and likely on mac OS right now is test_convert_deps_to_pip and test_convert_deps_to_pip_one_way. Still investigating how these failures started in the main branch, so if you see that specifically its safe to ignore. The other tests should all pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran with the main branch, but there are more failed tests on macOS other than test_convert_deps_to_pip and test_convert_deps_to_pip_one_way.

Copy link
Member

Choose a reason for hiding this comment

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

@dqkqd It is possible that the new version of setuptools that was released yesterday has affected things. I will re-run the main branch actions, but you can see the last time it ran upon merge ony those *_depts_to_pip tests had failed: https://github.com/pypa/pipenv/runs/7758310446?check_suite_focus=true

Copy link
Member

@matteius matteius Aug 13, 2022

Choose a reason for hiding this comment

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

@dqkqd I fixed the unexpected test failures upstream in main branch, in this PR: #5241

I am still not sure why those *_depts_to_pip tests have sporadic failures in the CI and Mac OS, but thats another can of worms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matteius I updated as you said. Not sure why it passed *_depts_to_pip cases. I didn't even modify convert_deps_to_pip.

Copy link
Member

Choose a reason for hiding this comment

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

@dqkqd It is a "flakey" test case, maybe I'll mark it as such and it won't fail the build as often. I haven't gotten to the bottom of why sometimes it behaves differently, it has failed on Mac OS most frequently, but I have seen it once in a while fail on other systems.

@@ -630,6 +660,37 @@ def parsed_constraints(self):
)
return self._parsed_constraints

@property
Copy link
Member

Choose a reason for hiding this comment

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

Recommend also making this a @cached_property

)
return self._parsed_default_constraints

@property
Copy link
Member

Choose a reason for hiding this comment

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

And this one also I think would be better as a cached_property.


@property
def default_constraints(self):
from pipenv.patched.pip._internal.req.constructors import (
Copy link
Member

Choose a reason for hiding this comment

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

Would be better if these imports were at the top of the files -- shouldn't be a circular import in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added that at the top of the file.

@@ -646,6 +707,8 @@ def constraints(self):
)
for c in self.parsed_constraints
]
if self.dev:
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain more why we only include the default_constraints when dev is specified? I don't think I quite understand out of the gate. I got to looking at this more because this is the only place that uses self.dev, and I am noticing how many parameters pip_install takes already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think constraint is used only when installing dev-packages. If it does with new normal packages, the installing package could not overwrite existing packages, because it use them as contraint, and therefore might not be installed.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok thanks, that makes sense.

@@ -552,6 +560,28 @@ def constraint_file(self):
self._constraint_file = self.prepare_constraint_file()
return self._constraint_file

def prepare_default_constraint_file(self):
Copy link
Member

Choose a reason for hiding this comment

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

This feels like something that could be a standalone utility in dependencies.py that is not method of the class that might be helpful towards future refactors. something like def prepare_default_constraint_file(directory, project)

Copy link
Member

Choose a reason for hiding this comment

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

Actually, on second thought, this feels very similar to the method get_constraints_from_deps -- Perhaps we could extract out the creation of the tracked tempfile and the writing to it of the constraints out to a shared method that takes the directory and the constraints to be written. Like def prepare_default_constraint_file(directory, constraints). It could be re-used by write_constraint_to_file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I will try this. Btw, I might not be able to response to this issue in the next few days. I’m sorry. Will get back on to this as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

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

It could also be re-used by prepare_constraint_file which has similar logic right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote standalone utility in dependencies.py, but not sure how to use it in prepare_constraint_file. Maybe my implementation is not good. Please tell me if there are any room for improvement.

Copy link
Member

Choose a reason for hiding this comment

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

@dqkqd Thanks, it looks great, basically I think you need to add a third parameter which is the constraints and have that computed outside the helper utility file, because that is what is different about the three places that write this file. I think if we can solve for that, it will be ready for merge :-D

Copy link
Member

Choose a reason for hiding this comment

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

Oh -- and please add a news fragment :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified it and add news fragment.

@matteius
Copy link
Member

@dqkqd I left a few more notes about areas I think we could further refine the code and reduce duplication. I think if we can factor out the three spots we write constraint files into a common utility and some of the places we can use @cached_property to allow for simpler and more readable properties. I think the change makes sense and could be merged, but chipping away at the some more of this type of technical debt would be good.

@@ -298,6 +298,31 @@ def is_constraint(dep):
return constraints


def prepare_default_constraint_file(
project,
dir=None,
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid shadowing built in python function name dir -- I recommend naming it directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it.

@matteius matteius merged commit e9dc324 into pypa:main Aug 13, 2022
@matteius
Copy link
Member

@dqkqd Nice work!

@dqkqd
Copy link
Contributor Author

dqkqd commented Aug 13, 2022

@matteius Thank you.

@dqkqd dqkqd deleted the issue-4371-incorrect-dependencies-when-install-dev-packages branch August 13, 2022 13:36
@oz123 oz123 mentioned this pull request Oct 24, 2022
yeisonvargasf pushed a commit to yeisonvargasf/pipenv that referenced this pull request Nov 19, 2022
* Add test, ensure dev lock use default packages as constraints.

* Use default packages as constraints when locking develop packages.

* Add test, ensure installing dev-packages use default packages as constraints. (pypa#4371) (pypa#2987)

* Use default packages as constraints when installing provided dev packages.

* change vistir.path.normalize_path to pipenv.utils.shell.normalize_path

* Add function that get contraints from packages.

* Add test for get_constraints_from_deps function

* Use get_constraints_from_deps to get constraints

* Use @cached_property instead of @Property

* Use standalone utility to write constraints file

* prepare_constraint_file use precomputed constraints.

* Add news fragment.
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.

Pipenv installs spurious dependencies
2 participants