Update patches for 1.7.0 #43

jjyao · 2021-12-03T19:28:07Z

Checklist

Used a personal fork of the feedstock to propose changes
Bumped the build number (if the version is unchanged)
Reset the build number to 0 (if the version changed)
Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
Ensured the license file is being packaged.

conda-forge-linter · 2021-12-03T19:28:11Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

h-vetinari · 2021-12-03T22:17:00Z

It looks like bazel doesn't like the following:

version_info = bazel_invoke(subprocess.check_output, ["--version"])

as it results in

[FATAL 19:45:39.671 src/main/cpp/blaze.cc:1293] Unknown startup option: '--version'.
  For more info, run 'bazel help startup_options'

h-vetinari · 2021-12-03T22:18:40Z

Windows looks even worse. I'd suggest to leave the bazel version untouched for now, unless absolutely necessary for upstream.

recipe/meta.yaml

h-vetinari · 2021-12-03T22:22:38Z

I had tried rebasing the patches for 1.7.0 myself, but ran into other problems. So far, @vnlitvinov and @krfricke haven't responded for comment.

I appreciate that you're giving this a shot - hopefully you get further than I do and we can unblock this!

vnlitvinov · 2021-12-06T16:27:53Z

My guess is this is too new Bazel version (I see upstream updated to 4.2.1, but maybe they did it later than 1.7.0 this tries to tackle).

As for Windows - it seems either directory layout was changed, or repository, or symlink->junction replacement was unsuccessful. Symlinks in git under Windows do not behave well...

vnlitvinov · 2021-12-09T17:03:59Z

recipe/patches/0007-Remove-bazel-version-check.patch

+-    bazel_version = tuple(map(int, bazel_version_digits))
+-    if bazel_version < SUPPORTED_BAZEL:
+-        logger.warning("Expected Bazel version {} but found {}".format(
+-            ".".join(map(str, SUPPORTED_BAZEL)), bazel_version_str))


I suggest you add the removal of SUPPORTED_BAZEL variable in this patch, too. It would make it so next time upstream changes Bazel version we'll have a merge conflict here which would prompt us to update the Bazel version in the recipe.

SUPPORTED_BAZEL is still needed in the setup.py. Also I'm removing that bazel version check code in the upstream as well: ray-project/ray#20990

My idea with removing the declaration was to create a conflict here in conda-forge when required Bazel version changes, so meta.yaml would be updated accordingly (kind of semi-automatic version check).

what about purposely creating a conflict when Bazel required version changes? so meta.yaml could be updated when this happens

I'd like to defer it to a separate PR: we need a solution not just for bazel but for all dependencies in meta.yaml. Currently I need to manually update them and make sure they are consistent with the versions in the upstream. We need a better solution here.

Consistency is semi-automatic with current solution - a merge conflict happens when something is changed in setup.py which touches the dependencies. Ideally this should be fully automatic, I totally agree, but I don't have better solution yet.

It would be awesome if you could come up with any better approach, I would gladly copy it to other conda packages I maintain 🙃

I'm not so worried about patching this out. If the recipe builds and passes the tests, then - ipso facto - the bazel version wasn't doing too badly. OTOH, if we're using a bazel version that's really not supported, things will fail loudly anyway.

That said, I don't think it's a bad idea what @vnlitvinov is suggesting - explicit is better than implicit, and as such we'd get a clear reminder (through a rebase conflict) if the supported bazel version changed. But it doesn't have to be done for this PR, IMO, let's just try to keep moving through the queue first.

Thanks for pushing on this BTW, @jjyao! :)

conda-forge-linter · 2021-12-20T19:28:58Z

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [65]

For recipe:

Recipes should usually depend on matplotlib-base as opposed to matplotlib so that runtime environments do not require large packages like qt.

conda-forge-linter · 2021-12-20T19:30:57Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

Recipes should usually depend on matplotlib-base as opposed to matplotlib so that runtime environments do not require large packages like qt.

jjyao · 2021-12-20T19:38:29Z

@h-vetinari I faced the same issue as you Python Configuration Error: Problem getting python include path for /home/conda/feedstock_root/build_artifacts/ray-packages_1639068846815/_build_env/bin/python.

One thing I noticed is that the python version in build_artifacts is python 3.10 not the target python version (3.6 - 3.9).

[root@8252aaf75e12 bin]# ls -lh python
lrwxrwxrwx 1 conda conda 10 Dec 20 18:51 python -> python3.10
[root@8252aaf75e12 bin]# pwd
/home/conda/feedstock_root/build_artifacts/ray-packages_1640025700006/_build_env/bin

Also I found the following in the output:

The following NEW packages will be INSTALLED:
    python:                   3.10.0-h543edf9_3_cpython conda-forge
...

The following NEW packages will be INSTALLED:
    python:           3.7.12-hf930737_100_cpython conda-forge
...

The following NEW packages will be INSTALLED:
    python:                               3.10.0-h543edf9_3_cpython conda-forge

I'm also reverting the bazel version to the old one and let's see if it works.

jjyao · 2021-12-20T19:51:30Z

Even if I reverted the bazel version, it's still the same error Python Configuration Error: Problem getting python include path for /home/conda/feedstock_root/build_artifacts/ray-packages_1639068846815/_build_env/bin/python.

jjyao · 2021-12-20T21:01:26Z

I think I found the issue:

The failure is from https://github.com/grpc/grpc/blob/master/third_party/py/python_configure.bzl:

def _get_python_include(repository_ctx, python_bin):
    """Gets the python include path."""
    result = _execute(
        repository_ctx,
        [
            python_bin,
            "-c",
            "from __future__ import print_function;" +
            "from distutils import sysconfig;" +
            "print(sysconfig.get_python_inc())",
        ],
        error_msg = "Problem getting python include path for {}.".format(python_bin),
        error_details = (
            "Is the Python binary path set up right? " + "(See ./configure or " +
            python_bin + ".) " + "Is distutils installed? " +
            _HEADERS_HELP
        ),
    )

The way how they check whether the command succeeds or not is by checking if stderr is empty (instead of exit code). With python 3.10, distutils is deprecated and we have

<string>:1: DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives
<string>:1: DeprecationWarning: The distutils.sysconfig module is deprecated, use sysconfig instead

in stderr which caused the command to fail.

jjyao · 2021-12-21T17:13:48Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2021.12.21.14.59.39

jjyao · 2021-12-21T17:55:38Z

Hmm, seems after rerender, the multi variant build is removed. Any ideas?

vnlitvinov · 2022-01-18T08:43:09Z

Symlink support is already enabled I believe, but symlinks on Windows are a very funky business.

I had made some post-checkout replacements with junctions if you take a look at setup.py:
https://github.com/ray-project/ray/blob/1315293dd8c524fea863c2b25a4ad2daa84351fc/python/setup.py#L368-L421

If this patching is not an option, I think disabling streaming-related things is the best way to go, especially if they're going to be dropped soon - no point in fixing what's going to be removed very soon.

jjyao · 2022-01-18T15:51:02Z

Hmm... The code you showed already exists in 1.7 not sure why it's not working.

vnlitvinov · 2022-01-18T16:12:35Z

This time it looks the setup.py needs to be updated, as the directory layout changed - there seem to no longer be a new_dashboard directory.

jjyao · 2022-01-18T17:26:22Z

Oh, yea new_dashboard is removed in 1.7.

jjyao · 2022-01-18T17:33:10Z

At least we confirmed that we thought conda environment automatically sets IS_AUTOMATED_BUILD but it's not.

jjyao · 2022-01-19T02:45:47Z

D:\bld\ray-packages_1642549985623\_h_env\lib\site-packages\setuptools\command\install.py:37: SetuptoolsDeprecationWarning: setup.py install is deprecated. Use build and pip and other standards-based tools.
  setuptools.SetuptoolsDeprecationWarning,
D:\bld\ray-packages_1642549985623\_h_env\lib\site-packages\setuptools\command\easy_install.py:159: EasyInstallDeprecationWarning: easy_install command is deprecated. Use build and pip and other standards-based tools.
  EasyInstallDeprecationWarning,
error: [WinError 3] The system cannot find the path specified: 'ray/streaming/generated'
Traceback (most recent call last):
  File "setup.py", line 364, in <module>
    replace_symlinks_with_junctions()
  File "setup.py", line 350, in replace_symlinks_with_junctions
    raise ValueError(f"Unexpected type of entry: '{path}'")
ValueError: Unexpected type of entry: 'ray\rllib'

Hmm... this symlink should be there.

vnlitvinov · 2022-01-19T08:50:22Z

I have noticed just now that this line is incorrect:
https://github.com/ray-project/ray/blob/1315293dd8c524fea863c2b25a4ad2daa84351fc/python/setup.py#L419

It uses single quotes which Windows cmd.exe does not interpret correctly, this might be a reason why things fail here.

jjyao · 2022-01-19T16:15:39Z

@vnlitvinov Are you saying we should do f"MKLINK /J \"{os.path.basename(link)}\" \"{target}\""? Although the failure is before this line.

Also for our windows pip package, we don't run replace_symlinks_with_junctions but just set git config --global core.symlinks true before checking out the code, can we config the same thing in conda environment?

vnlitvinov · 2022-01-19T16:54:44Z

It should be easier for the reader if it's written as f'MKLINK /J "{os.path.basename(link)}" "{target}"' (as Python doesn't care about single or double quotes).

I don't know about configuring git in conda build, this needs some research.

jjyao · 2022-01-19T17:54:51Z

I changed the source from a zip file to a git repo, it worked for linux but failed for windows:

Cloning into bare repository '/d/bld/git_cache/github.com/ray-project/ray.git'...
Traceback (most recent call last):
  File "C:\Miniconda\Scripts\conda-mambabuild-script.py", line 9, in <module>
    sys.exit(main())
  File "C:\Miniconda\lib\site-packages\boa\cli\mambabuild.py", line 215, in main
    call_conda_build(action, config)
  File "C:\Miniconda\lib\site-packages\boa\cli\mambabuild.py", line 187, in call_conda_build
    result = api.build(
  File "C:\Miniconda\lib\site-packages\conda_build\api.py", line 186, in build
    return build_tree(
  File "C:\Miniconda\lib\site-packages\conda_build\build.py", line 3083, in build_tree
    packages_from_this = build(metadata, stats,
  File "C:\Miniconda\lib\site-packages\conda_build\build.py", line 2133, in build
    try_download(m, no_download_source=False, raise_error=True)
  File "C:\Miniconda\lib\site-packages\conda_build\render.py", line 649, in try_download
    source.provide(metadata)
  File "C:\Miniconda\lib\site-packages\conda_build\source.py", line 863, in provide
    git = git_source(source_dict, metadata.config.git_cache, src_dir, metadata.path,
  File "C:\Miniconda\lib\site-packages\conda_build\source.py", line 328, in git_source
    git_mirror_checkout_recursive(
  File "C:\Miniconda\lib\site-packages\conda_build\source.py", line 248, in git_mirror_checkout_recursive
    assert isdir(mirror_dir)
AssertionError

@jaimergp any ideas?

jaimergp · 2022-01-19T18:44:24Z

Maybe something to do with git_depth? https://docs.conda.io/projects/conda-build/en/latest/resources/define-metadata.html#source-from-git

it's an error during the git clone, but no idea why. Why do you need git as a source, instead of the tarball?

jjyao · 2022-01-19T22:19:01Z

I'm trying to see if git clone fixes the symlink issue for us on windows. I'll revert back to tarball.

jjyao · 2022-01-20T03:25:09Z

Finally, everything has passed. Could we merge it? @h-vetinari @vnlitvinov @krfricke

vnlitvinov

Finally! Congratulations to everyone involved!

recipe/patches/0008-Fix-C-17-support-on-windows.patch

vnlitvinov · 2022-01-20T07:47:49Z

recipe/patches/0009-Fix-replace_symlinks_with_junctions.patch

+-        r"ray\new_dashboard": "../../dashboard",
+        r"ray\dashboard": "../../dashboard",
+         r"ray\rllib": "../../rllib",
+         r"ray\streaming": "../../streaming/python/",
+     }
+@@ -353,7 +353,7 @@ def replace_symlinks_with_junctions():
+                 os.path.join(os.path.dirname(path), target))
+             logger.info("Setting {} -> {}".format(link, target))
+             subprocess.check_call(
+-                f"MKLINK /J '{os.path.basename(link)}' '{target}'",
+                f'MKLINK /J "{os.path.basename(link)}" "{target}"',


and these changes should also be upstreamed, I think

vnlitvinov · 2022-01-20T07:58:11Z

@jjyao thanks again for pushing this, I'll leave a day or so for others to review and, unless there would be no objections, I'm planning to merge this in ~24h.

h-vetinari

Currently travelling without a laptop, but the bulk looks good, and I'm happy with a green CI already. 👍

Since the commits are almost all named the same, I'd be in favour of a squash merge (or creating a more reasonable commit history, for example: all changes in recipe/, and a separate commit for the feedstock rerender)

h-vetinari · 2022-01-20T10:04:39Z

recipe/meta.yaml

      host:
-        - nodejs
+        - nodejs <16.6.2


This will need to be lifted again sometime soon-ish.

h-vetinari · 2022-01-20T10:07:12Z

recipe/conda_build_config.yaml

@@ -6,3 +6,8 @@ cudnn:                                          # [linux64]
  - undefined                                   # [linux64]
 cdt_name:                                       # [linux64]
  - cos7                                        # [linux64]
+
+c_compiler:                                     # [win]
+- vs2019                                        # [win]


Just to make sure (after adding, reverting, etc.), this now upgrades the windows compilers. I don't mind doing that (actually, even in favour since the death of vs2017 is hastened greatly by msft currently), but just wanted to check if this is now the intention.

It's intentional since upstream also uses vs2019.

Windows cmd.exe doesn't interpret single quote correctly. See conda-forge/ray-packages-feedstock#43

Update patches for 1.7.0

48d0863

jjyao requested review from dHannasch, h-vetinari, krfricke and vnlitvinov as code owners December 3, 2021 19:28

h-vetinari reviewed Dec 3, 2021

View reviewed changes

recipe/meta.yaml Show resolved Hide resolved

Update patches for 1.7.0

75e66c8

vnlitvinov reviewed Dec 9, 2021

View reviewed changes

Update patches for 1.7.0

45253eb

Update patches for 1.7.0

cb80fa9

Update patches for 1.7.0

e2b0a52

jjyao added 2 commits December 21, 2021 07:37

Update patches for 1.7.0

0f00018

Update patches for 1.7.0

9bfb167

MNT: Re-rendered with conda-build 3.21.7, conda-smithy 3.16.0, and co…

ab2554f

…nda-forge-pinning 2021.12.21.14.59.39

jjyao requested review from h-vetinari and vnlitvinov December 21, 2021 19:42

jjyao added 2 commits January 3, 2022 07:25

Update patches for 1.7.0

143617f

Update patches for 1.7.0

43d2fb3

Set IS_AUTOMATED_BUILD to 1

44a084c

jjyao added 3 commits January 18, 2022 09:47

Update patches for 1.7.0

1048a9d

Update patches for 1.7.0

f65aa49

Set IS_AUTOMATED_BUILD to 1

16841a1

Debug

b1eede8

Git clone

c4c1cbb

jjyao added 3 commits January 19, 2022 14:19

Use tar file as source

91e82e4

Update patches for 1.7.0

4455f1b

Remove streaming patch

91a935a

jjyao mentioned this pull request Jan 20, 2022

Fix replace_symlinks_with_junctions for windows ray-project/ray#21720

Merged

6 tasks

vnlitvinov approved these changes Jan 20, 2022

View reviewed changes

h-vetinari approved these changes Jan 20, 2022

View reviewed changes

ericl pushed a commit to ray-project/ray that referenced this pull request Jan 20, 2022

Fix replace_symlinks_with_junctions for windows (#21720)

fa9feb5

Windows cmd.exe doesn't interpret single quote correctly. See conda-forge/ray-packages-feedstock#43

vnlitvinov merged commit 93c846c into conda-forge:master Jan 21, 2022

jjyao deleted the jjyao/1.7.0 branch January 21, 2022 20:36

vnlitvinov mentioned this pull request Jan 24, 2022

ray-packages v1.7.0 #39

Closed

3 tasks

Update patches for 1.7.0 #43

Update patches for 1.7.0 #43

Conversation

jjyao commented Dec 3, 2021

conda-forge-linter commented Dec 3, 2021

h-vetinari commented Dec 3, 2021

h-vetinari commented Dec 3, 2021

h-vetinari commented Dec 3, 2021

vnlitvinov commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnlitvinov Jan 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conda-forge-linter commented Dec 20, 2021

conda-forge-linter commented Dec 20, 2021

jjyao commented Dec 20, 2021

jjyao commented Dec 20, 2021

jjyao commented Dec 20, 2021

jjyao commented Dec 21, 2021

jjyao commented Dec 21, 2021

vnlitvinov commented Jan 18, 2022

jjyao commented Jan 18, 2022

vnlitvinov commented Jan 18, 2022

jjyao commented Jan 18, 2022

jjyao commented Jan 18, 2022

jjyao commented Jan 19, 2022

vnlitvinov commented Jan 19, 2022

jjyao commented Jan 19, 2022 • edited

vnlitvinov commented Jan 19, 2022

jjyao commented Jan 19, 2022

jaimergp commented Jan 19, 2022

jjyao commented Jan 19, 2022

jjyao commented Jan 20, 2022

vnlitvinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnlitvinov commented Jan 20, 2022

h-vetinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnlitvinov Jan 13, 2022 •

edited

jjyao commented Jan 19, 2022 •

edited