Ray 1.4.1 #28

krfricke · 2021-08-06T12:01:23Z

Build passes locally

Closes #20

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-08-06T12:01:27Z

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-08-06T12:08:45Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2021.08.05.18.49.42

vnlitvinov

Thanks @krfricke for taking this up!
Please update requirements in meta.yaml, and we should be good to go when CI passes.

recipe/patches/0010-Empty-install-requirements-installed-by-conda-build.patch

recipe/patches/0013-Add-bazel-linkopts-libs.patch

h-vetinari

Thanks for taking this up!

The rerender I triggered was so that you actually become a maintainer when this PR is merged. 🙃

recipe/patches/0010-Empty-install-requirements-installed-by-conda-build.patch

krfricke · 2021-08-06T12:41:05Z

Thanks for your help reviewing this! Updated the dependencies in the meta.yaml

h-vetinari · 2021-08-06T12:42:26Z

Thanks for your help reviewing this! Updated the dependencies in the meta.yaml

Did you see my comments? This is a misspecification (cython is debatable, but numpy certainly). I'd ask you to revert (or rebase out) that commit.

krfricke · 2021-08-06T12:51:22Z

There are CI failures that seem to be unrelated:

+ conda install --yes --quiet conda-forge-ci-setup=3 conda-build pip -c conda-forge
Collecting package metadata (current_repodata.json): ...working... 
CondaHTTPError: HTTP 000 CONNECTION FAILED for url <https://conda.anaconda.org/conda-forge/noarch/current_repodata.json>
Elapsed: -

An HTTP error occurred when trying to retrieve this URL.
HTTP errors are often intermittent, and a simple retry will get you on your way.
'https://conda.anaconda.org/conda-forge/noarch'

can we re-trigger those tests?

krfricke · 2021-08-06T12:53:05Z

In this commit: fbb74dc
I updated the meta to reflect the dependencies from the setup.py - I thought that was what you meant, or did I misunderstand?

krfricke · 2021-08-06T12:54:22Z

Ah sorry I became confused by the comments. Happy to revert the commit

This reverts commit fbb74dc.

vnlitvinov · 2021-08-06T13:16:03Z

@h-vetinari do you know what's going on? seems like something very low in the dependency chain had updated glibc (I wonder what it was), but the image used here does not have new enough glibc...

h-vetinari · 2021-08-06T13:31:12Z

Can you try adding - sysroot_linux-64 2.17 # [linux64] to the build requirement of the failing package (presumably ray-core)?

h-vetinari · 2021-08-06T13:33:03Z

Ah, and we probably will need something like

cdt_name:  # [linux]
  - cos7   # [linux]

in conda_build_config.yaml (needs to be created; then rerendered)

vnlitvinov · 2021-08-06T13:44:19Z

I am still wary about moving to cos7 as I saw compatibility issues with other packages - is it now the default version in conda-forge?

krfricke · 2021-08-06T13:56:45Z

I added sysroot linux - let me know about cos7

h-vetinari · 2021-08-06T13:58:55Z

I am still wary about moving to cos7 as I saw compatibility issues with other packages - is it now the default version in conda-forge?

CentOS 6 is EOL since more than half a year. conda-forge is still discussing about migrating (conda-forge/conda-forge.github.io#1436) but most people are ready to do it.

The compatibility issues should be simple (and mostly in conda-forge CI) - you cannot be on cos6 and also depend on a package built against glibc 2.17.

By adding the nodejs-dependency, we won't be able to avoid it here: https://github.com/conda-forge/nodejs-feedstock/blob/master/recipe/meta.yaml#L33

h-vetinari · 2021-08-06T13:59:34Z

I added sysroot linux - let me know about cos7

It's probably gonna fail because the default image does not have a new enough glibc to satisfy that constraint.

krfricke · 2021-08-06T14:05:48Z

@conda-forge-admin, please rerender

github-actions · 2021-08-06T14:06:45Z

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you but ran into some issues, please ping conda-forge/core
for further assistance. You can also try re-rendering locally.

krfricke · 2021-08-06T14:14:37Z

subspace that is not fully implemented. To be clear:
.. we did not find ['cos7'] from {'cdt_name': ['cos7']} in cdt_name:['cos6', 'cos6']

Not familiar with this config, any pointers? I'll try to look this up in the meantime

h-vetinari · 2021-08-06T15:12:24Z

Not familiar with this config, any pointers? I'll try to look this up in the meantime

Sorry, I always forget the details; let me fix it.

h-vetinari · 2021-08-06T15:12:40Z

Do you mind if I push into your PR?

krfricke · 2021-08-06T17:31:32Z

Not at all, please go ahead!

Co-Authored-By: H. Vetinari <h.vetinari@gmx.com>

…nda-forge-pinning 2021.08.05.18.49.42

h-vetinari

Linux passes, hopefully windows will too; LGTM (though I realized too late that the build number should ideally still be reset to 0)

h-vetinari · 2021-08-06T22:14:53Z

recipe/patches/0010-Empty-install-requirements-installed-by-conda-build.patch

-From 3c5886d8d47152735536fc7809908bbbad8041dd Mon Sep 17 00:00:00 2001
-From: Vasily Litvinov <vasilij.n.litvinov@intel.com>
-Date: Fri, 4 Dec 2020 12:49:34 +0300
+From 353efadf23e790e831f87021ae5d4517331e9a2d Mon Sep 17 00:00:00 2001
+From: Kai Fricke <kai@anyscale.com>
+Date: Fri, 6 Aug 2021 11:36:40 +0100


Sidenote: we try to keep the attribution to the original authors when rebasing patches; less for the glory and more for knowing who to ask about it ;-)

Ah sorry! I just git am'd everything and created new patches with git format-patch which seems to have lost the original authors. I'll try to sort this out

Ah so this was only in this patch, and because I actually didn't git am it after all (because of conflicts) but manually did the required changes again and committed with the same commit message. What is the best way to go about this next time?

What is the best way to go about this next time?

For me personally, I keep a branch in a local copy of the upstream repo that I rebase onto new versions, and if a conflict occurs, I try to use git commit -m "<original_message>" --author="<original_author>" --date="<original_date>" after resolving possible conflicts (if none exist, it'll be fine already).

# Conflicts: # recipe/conda_build_config.yaml

vnlitvinov

Mostly good, so stamping approval.
Would like to see my questions answered, though.

vnlitvinov · 2021-08-09T11:21:52Z

recipe/conda_build_config.yaml

+cuda_compiler_version:                          # [linux64]
+  - None                                        # [linux64]
+docker_image:                                   # [linux64]
+  - quay.io/condaforge/linux-anvil-cos7-x86_64  # [linux64]


so there is no way to stay on cos6?

No, we cannot, see this comment, specifically:

By adding the nodejs-dependency, we won't be able to avoid it here: https://github.com/conda-forge/nodejs-feedstock/blob/master/recipe/meta.yaml#L33

But we can use older nodejs, e.g. one from 14.x version part, which is still in "active" phase according to their schedule and works on cos6 :)

Ah, this comment didn't show up in the main thread for some reason.

I'm strongly against pinning to old versions for the sake of EOL distros.

If you feel so strongly about cos6-support, we can add a branch where nodejs is pinned to 14.x, but master must move on. (though this would need some care with the build numbers; e.g. build numbers on master start from 100)

vnlitvinov · 2021-08-09T11:22:28Z

recipe/meta.yaml

@@ -71,6 +71,7 @@ outputs:
        - {{ compiler('cxx') }}
        - bazel <=3.4.1  # [not win]
        - bazel <3.7     # [win]
+        - sysroot_linux-64 2.17  # [linux64]


if we update the base to cos7 do we need to manually add this?..

It's necessary so that this package gets the correct run-export (so that packages depending on ray fail to build if they don't have cos7 themselves), otherwise we might get crashes with the ABI.

vnlitvinov · 2021-08-09T11:22:38Z

recipe/meta.yaml

@@ -305,3 +306,4 @@ extra:
    - dHannasch
    - h-vetinari
    - vnlitvinov
+    - krfricke


welcome on board! 🙃

Thanks 😄

Agreed, already mentioned that on the last PR - happy to have you on board!

krfricke · 2021-08-09T12:29:30Z

FYI I prepared the update for Ray 1.5.0 based on this revision and will open the PR once this has been merged

h-vetinari

Let's put this in 🙃

h-vetinari · 2021-08-09T12:47:16Z

recipe/conda_build_config.yaml

+cuda_compiler_version:                          # [linux64]
+  - None                                        # [linux64]
+docker_image:                                   # [linux64]
+  - quay.io/condaforge/linux-anvil-cos7-x86_64  # [linux64]


No, we cannot, see this comment, specifically:

By adding the nodejs-dependency, we won't be able to avoid it here: https://github.com/conda-forge/nodejs-feedstock/blob/master/recipe/meta.yaml#L33

h-vetinari · 2021-08-09T12:48:43Z

recipe/meta.yaml

@@ -71,6 +71,7 @@ outputs:
        - {{ compiler('cxx') }}
        - bazel <=3.4.1  # [not win]
        - bazel <3.7     # [win]
+        - sysroot_linux-64 2.17  # [linux64]


It's necessary so that this package gets the correct run-export (so that packages depending on ray fail to build if they don't have cos7 themselves), otherwise we might get crashes with the ABI.

h-vetinari · 2021-08-09T12:49:03Z

recipe/meta.yaml

@@ -305,3 +306,4 @@ extra:
    - dHannasch
    - h-vetinari
    - vnlitvinov
+    - krfricke


Agreed, already mentioned that on the last PR - happy to have you on board!

vnlitvinov · 2021-08-09T13:06:10Z

@h-vetinari I've removed "automerge" label because I want to finish the cos6 vs cos7 discussion first.

h-vetinari · 2021-08-09T14:11:17Z

@h-vetinari I've removed "automerge" label because I want to finish the cos6 vs cos7 discussion first.

Unless you can remove the nodejs dependency, there is no wiggle room on this.

h-vetinari · 2021-08-09T14:14:11Z

I don't understand the problem though - cos6 is EOL for almost a year, glibc 2.12 is a decade old. There's a vanishingly small fraction of users affected by moving to cos7.

The only pain is having to build all packages dependent on ray with cos7 as well, but that's manageable IMO. Also conda-forge will move to cos7 in the foreseeable future (see link above), it just takes a bit longer when it's on such a scale.

h-vetinari · 2021-08-09T23:44:52Z

@vnlitvinov
I'd like to merge this and make our way towards 1.5.1. If you want to have a branch for cos6 (pinning old nodejs), we can do that, but should then adapt the build number scheme before merging (the higher ones being reserved for master branch). Obviously, I'd prefer to avoid that hassle and just require cos7, because - as laid out above - I really don't see the issue with doing that.

vnlitvinov · 2021-08-10T10:37:52Z

Okay, let's be bold and just move to cos7 - on our way to the shiny future! Thanks @krfricke again for your efforts and for taking this up!

Kai Fricke added 4 commits August 6, 2021 10:04

Ray 1.4.1

c65c000

Fix sha

f356865

Update patches

59f28c8

Add bazel linkopts/libs patch

440a2ff

krfricke requested review from dHannasch, h-vetinari and vnlitvinov as code owners August 6, 2021 12:01

Fix patch nubers

a50fbf4

MNT: Re-rendered with conda-build 3.21.4, conda-smithy 3.11.0, and co…

ed2ea02

…nda-forge-pinning 2021.08.05.18.49.42

vnlitvinov requested changes Aug 6, 2021

View reviewed changes

recipe/patches/0010-Empty-install-requirements-installed-by-conda-build.patch Show resolved Hide resolved

recipe/patches/0010-Empty-install-requirements-installed-by-conda-build.patch Show resolved Hide resolved

recipe/patches/0013-Add-bazel-linkopts-libs.patch Show resolved Hide resolved

h-vetinari reviewed Aug 6, 2021

View reviewed changes

recipe/patches/0010-Empty-install-requirements-installed-by-conda-build.patch Show resolved Hide resolved

recipe/patches/0010-Empty-install-requirements-installed-by-conda-build.patch Show resolved Hide resolved

Update meta file

fbb74dc

Revert "Update meta file"

7ca68ba

This reverts commit fbb74dc.

sysroot_linux

0ad6110

onda build config / cos7

b4111ef

sysroot_linux

c4d81d8

Kai Fricke and others added 2 commits August 6, 2021 22:47

conda build config / cos7

ce86c2b

Co-Authored-By: H. Vetinari <h.vetinari@gmx.com>

MNT: Re-rendered with conda-build 3.21.4, conda-smithy 3.11.0, and co…

28a2855

…nda-forge-pinning 2021.08.05.18.49.42

h-vetinari force-pushed the ray-1.4.1 branch from b4111ef to 28a2855 Compare August 6, 2021 20:48

h-vetinari approved these changes Aug 6, 2021

View reviewed changes

h-vetinari requested a review from vnlitvinov August 6, 2021 21:59

h-vetinari reviewed Aug 6, 2021

View reviewed changes

Merge remote-tracking branch 'origin/ray-1.4.1' into ray-1.4.1

0e9a7ea

# Conflicts: # recipe/conda_build_config.yaml

vnlitvinov approved these changes Aug 9, 2021

View reviewed changes

Delete obsolete patch

9b99dcd

h-vetinari reviewed Aug 9, 2021

View reviewed changes

h-vetinari added the automerge Merge the PR when CI passes label Aug 9, 2021

vnlitvinov removed the automerge Merge the PR when CI passes label Aug 9, 2021

vnlitvinov merged commit 31dca52 into conda-forge:master Aug 10, 2021

krfricke deleted the ray-1.4.1 branch August 10, 2021 19:09

h-vetinari mentioned this pull request Jan 10, 2023

Unpin nodejs (build for 14/16/18) #89

Merged

Ray 1.4.1 #28

Ray 1.4.1 #28

Conversation

krfricke commented Aug 6, 2021 • edited by h-vetinari Loading

conda-forge-linter commented Aug 6, 2021

h-vetinari commented Aug 6, 2021

vnlitvinov left a comment

Choose a reason for hiding this comment

h-vetinari left a comment

Choose a reason for hiding this comment

krfricke commented Aug 6, 2021

h-vetinari commented Aug 6, 2021

krfricke commented Aug 6, 2021

krfricke commented Aug 6, 2021

krfricke commented Aug 6, 2021

vnlitvinov commented Aug 6, 2021

h-vetinari commented Aug 6, 2021

h-vetinari commented Aug 6, 2021

vnlitvinov commented Aug 6, 2021

krfricke commented Aug 6, 2021

h-vetinari commented Aug 6, 2021

h-vetinari commented Aug 6, 2021

krfricke commented Aug 6, 2021

github-actions bot commented Aug 6, 2021

krfricke commented Aug 6, 2021

h-vetinari commented Aug 6, 2021

h-vetinari commented Aug 6, 2021

krfricke commented Aug 6, 2021

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari Aug 6, 2021 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-vetinari Aug 9, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krfricke commented Aug 9, 2021

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnlitvinov commented Aug 9, 2021

h-vetinari commented Aug 9, 2021

h-vetinari commented Aug 9, 2021

h-vetinari commented Aug 9, 2021

vnlitvinov commented Aug 10, 2021

krfricke commented Aug 6, 2021 •

edited by h-vetinari

Loading

h-vetinari Aug 6, 2021 •

edited

Loading

h-vetinari Aug 9, 2021 •

edited

Loading

h-vetinari Aug 9, 2021 •

edited

Loading

h-vetinari Aug 9, 2021 •

edited

Loading