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

TST Update pytest to 7.0 #23444

Merged
merged 8 commits into from May 30, 2022
Merged

TST Update pytest to 7.0 #23444

merged 8 commits into from May 30, 2022

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Closes #22396

What does this implement/fix? Explain your changes.

The test now works with pytest 7.0 without error.

Any other comments?

There is an error with pip-tools + pip 22.1 that is being tracked here: jazzband/pip-tools#1623

@ogrisel
Copy link
Member

ogrisel commented May 24, 2022

LGTM but please let me merge main and re-run python build_tools/update_environments_and_lock_files.py again now that #23429 has been merged.

@lesteve
Copy link
Member

lesteve commented May 24, 2022

There is an error with pip-tools + pip 22.1 that is being tracked here: jazzband/pip-tools#1623

It has been fixed in pip-tools 6.6.2: jazzband/pip-tools#1624

Comment on lines 7 to 8
--index-url https://d1yxz45j0ypngg.cloudfront.net/
--extra-index-url https://pypi.org/simple
Copy link
Member Author

Choose a reason for hiding this comment

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

@ogrisel This is strange. I do not think the nogil index should appear in the 32bit lock file?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch, this one is weird, I am going to guess this is because @ogrisel is using a nogil Python as his default Python 😉

Copy link
Member

Choose a reason for hiding this comment

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

That's weird. Let me git clean -xdf and try again.

Copy link
Member

@lesteve lesteve May 24, 2022

Choose a reason for hiding this comment

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

I don't really understand, because in principle the script creates environment to call pip-compile from so they should contain normal Python:

def write_pip_lock_file(build_metadata):
build_name = build_metadata["build_name"]
python_version = build_metadata["python_version"]
environment_name = f"pip-tools-python{python_version}"
# To make sure that the Python used to create the pip lock file is the same
# as the one used during the CI build where the lock file is used, we first
# create a conda environment with the correct Python version and
# pip-compile and run pip-compile in this environment
command = (
"conda create -c conda-forge -n"
f" pip-tools-python{python_version} python={python_version} pip-tools -y"
)
execute_command(shlex.split(command))
json_output = execute_command(shlex.split("conda info --json"))
conda_info = json.loads(json_output)
environment_folder = [
each for each in conda_info["envs"] if each.endswith(environment_name)
][0]
environment_path = Path(environment_folder)
pip_compile_path = environment_path / "bin" / "pip-compile"
folder_path = Path(build_metadata["folder"])
requirement_path = folder_path / f"{build_name}_requirements.txt"
lock_file_path = folder_path / f"{build_name}_lock.txt"
pip_compile(pip_compile_path, requirement_path, lock_file_path)

Copy link
Member

@lesteve lesteve May 24, 2022

Choose a reason for hiding this comment

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

Also it is very weird that this issue appears in a single pip lock file, and not the others ... if it was a pip configuration file or an environment variable it would appear in all the pip lock files ...

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand either, I cannot find where this custom index-url comes from. I did not use my nogil-venv when running this command.

I also tried to manually activate the conda env used to run pip-compile for the debian CI entry and there is not custom pip config in it:

(base) $ conda activate pip-tools-python3.9.2
(pip-tools-python3.9.2) $ pip config debug
env_var:
env:
global:
  /etc/xdg/xdg-ubuntu-xorg/pip/pip.conf, exists: False
  /etc/xdg/pip/pip.conf, exists: False
  /etc/pip.conf, exists: False
site:
  /home/ogrisel/mambaforge/envs/dev/envs/pip-tools-python3.9.2/pip.conf, exists: False
user:
  /home/ogrisel/.pip/pip.conf, exists: False
  /home/ogrisel/.config/pip/pip.conf, exists: False

Copy link
Member

Choose a reason for hiding this comment

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

I printed the command executed by the script and it is:

/home/ogrisel/mambaforge/envs/dev/envs/pip-tools-python3.9.2/bin/pip-compile --upgrade build_tools/azure/debian_atlas_32bit_requirements.txt -o build_tools/azure/debian_atlas_32bit_lock.txt

If I delete the build_tools/azure/debian_atlas_32bit_lock.txt and regenerate it by running the command manually, I still get the custom --index-url ...

Copy link
Member

Choose a reason for hiding this comment

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

@thomasjpfan feel free to re-run the command and git commit / push to this PR to fix it because I don't know how to fix my local setup.

@thomasjpfan
Copy link
Member Author

With the latest run, the diff got pretty big. It seems like the order changed drastically.

@lesteve
Copy link
Member

lesteve commented May 24, 2022

This is weird, were you using conda-lock 1.0.5 ? There were some bugs fixed regarding inconsistent ordering changes between two lockfile generations before 1.0.5 e.g. conda/conda-lock#170.

I regenerated the lock files on my machine, pushed the change and the diff seems more reasonable.

@lesteve
Copy link
Member

lesteve commented May 25, 2022

I forgot to say but in my last commit, I removed the pip<22.1 pin since pip-tools has fixed the issue as mentioned in #23444 (comment).

@ogrisel
Copy link
Member

ogrisel commented May 30, 2022

@thomasjpfan shall we merge? Have you checked the version of conda-lock you were using?

@thomasjpfan
Copy link
Member Author

I can not reproduce the big diff anymore even starting from 1a63adb (#23444) using conda-lock 1.0.5. It's possible I was using the wrong version the last time I generated the lock files.

I think this PR is good to merge.

@ogrisel
Copy link
Member

ogrisel commented May 30, 2022

Great, let's merge then.

@ogrisel ogrisel merged commit 008ab74 into scikit-learn:main May 30, 2022
ogrisel added a commit to ogrisel/scikit-learn that referenced this pull request Jul 11, 2022
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Aug 4, 2022
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
glemaitre pushed a commit that referenced this pull request Aug 5, 2022
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
mathijs02 pushed a commit to mathijs02/scikit-learn that referenced this pull request Dec 27, 2022
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Loïc Estève <loic.esteve@ymail.com>
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.

Enable pytest to work with >=7.0
3 participants