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

Updated build recipe and patches to ray-1.3.0 #14

Merged
merged 8 commits into from
Jun 22, 2021

Conversation

gshimansky
Copy link
Contributor

@gshimansky gshimansky commented Jun 11, 2021

Signed-off-by: Gregory Shimansky gregory.shimansky@intel.com

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
Copy link

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.

@gshimansky
Copy link
Contributor Author

@conda-forge-admin, please rerender

@gshimansky gshimansky marked this pull request as draft June 11, 2021 21:32
@github-actions
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.

@gshimansky gshimansky marked this pull request as ready for review June 16, 2021 00:50
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Would like to have @vnlitvinov's sign-off as well, but overall LGTM (did not verify the dependency changes with upstream, nor review the redis patch).

Comment on lines +22 to +25
+ #"rllib=ray.rllib.scripts:cli [rllib]",
+ #"tune=ray.tune.scripts:cli",
+ #"ray-operator=ray.ray_operator.operator:main",
+ #"serve=ray.serve.scripts:cli",
Copy link
Member

Choose a reason for hiding this comment

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

I feel we should be setting those entry points in the appropriate sub-packages.

Copy link
Contributor

Choose a reason for hiding this comment

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

either that (if they need special requirements and those subpackages provide those) or allow them be in ray-core

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This patch is taken verbatim from ray-1.2.0, no changes done to how these subpackages were handled in version 1.2.0
https://github.com/conda-forge/ray-packages-feedstock/blob/master/recipe/patches/0005-Disable-making-non-core-entry-scripts.patch

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added serve entry point to ray-serve. I suggest keeping ray-operator removed for now as it's unclear to which package it should belong. We can wait for someone coming and raising an issue I think :)

@h-vetinari
Copy link
Member

Thanks for working on this @gshimansky!

@h-vetinari
Copy link
Member

Ping @vnlitvinov
Would love to still have your input, otherwise merging in ~24h.

@gshimansky
Copy link
Contributor Author

@h-vetinari @vnlitvinov is currently on vacations and probably will answer only on the next week.

@h-vetinari
Copy link
Member

@gshimansky: @vnlitvinov is currently on vacations and probably will answer only on the next week.

OK, thanks for the info. One thing that would make me much more comfortable in merging this is if we were running the test suite on the build - see #5. If you could tackle that as well, it would be amazing. 🙃

@gshimansky
Copy link
Contributor Author

@h-vetinari yes you are right, using Ray tests to verify the build would be great. I saw #5 and will keep it in mind.

Copy link
Contributor

@vnlitvinov vnlitvinov left a comment

Choose a reason for hiding this comment

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

LGTM, I will be happy when the comment about separate entrypoints is addressed.

Comment on lines +22 to +25
+ #"rllib=ray.rllib.scripts:cli [rllib]",
+ #"tune=ray.tune.scripts:cli",
+ #"ray-operator=ray.ray_operator.operator:main",
+ #"serve=ray.serve.scripts:cli",
Copy link
Contributor

Choose a reason for hiding this comment

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

either that (if they need special requirements and those subpackages provide those) or allow them be in ray-core

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

This needs a merge or rebase to fix the build_num back to 0.

Otherwise LGTM, thanks!

@gshimansky
Copy link
Contributor Author

@h-vetinari
Copy link
Member

Isn't it 0 already? https://github.com/conda-forge/ray-packages-feedstock/pull/14/files#diff-f3725a55bf339595bf865fec73bda8ac99f283b0810c205442021f29c06eea9aR27

That's exactly the issue (diff not updated because no merge or rebase) - this is the state on master:

gshimansky and others added 7 commits June 21, 2021 21:13
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
which contains symlinks. Updated patches.

Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Gregory Shimansky <gregory.shimansky@intel.com>
Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>
@h-vetinari h-vetinari added the automerge Merge the PR when CI passes label Jun 21, 2021
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

Thanks again!

@h-vetinari
Copy link
Member

Seems ray.metrics_agent has been removed?

Traceback (most recent call last):
  File "/home/conda/feedstock_root/build_artifacts/ray-packages_1624303001859/test_tmp/run_test.py", line 23, in <module>
    import ray.metrics_agent
ModuleNotFoundError: No module named 'ray.metrics_agent'

@h-vetinari h-vetinari removed the automerge Merge the PR when CI passes label Jun 21, 2021
@h-vetinari
Copy link
Member

I'd be fine with removing that import-test (including comment), but would be nice if someone could confirm the upstream API break (removing an importable subpackage) - WDYT @vnlitvinov?

@gshimansky
Copy link
Contributor Author

Looking at Ray diff between version 1.2.0 and 1.3.0 it seems like they moved metrics_agent.py into _private package.
https://github.com/ray-project/ray/compare/ray-1.2.0..ray-1.3.0

@vnlitvinov
Copy link
Contributor

I'd be fine with removing that import-test (including comment), but would be nice if someone could confirm the upstream API break (removing an importable subpackage) - WDYT @vnlitvinov?

@richardliaw could you please comment?..

@richardliaw
Copy link

@vnlitvinov sorry having a bit of trouble following - are you asking to verify that the metrics agent is changes?

@vnlitvinov
Copy link
Contributor

@richardliaw
We've added the check to import ray.metrics_agent to the package CI to make sure we check the Ray initialization pipeline (ref: #16). Is it correct that the ray.metrics_agent is now moved? How should we check that what we build is in a working state?

@h-vetinari
Copy link
Member

How should we check that what we build is in a working state?

This because ray.init() seemed to do different things in our CI than for user installs (cf #16). Note though that this is just a baby step - I'd much rather run the full test suite (#5), but it's not clear to me how that should be called. I've asked on the upstream tracker already - would be very helpful if you could support there @richardliaw.

Signed-off-by: Vasily Litvinov <vasilij.n.litvinov@intel.com>
@vnlitvinov
Copy link
Contributor

While we're waiting for a better solution for testing the packages I've changed the import tests to check for ray._private.metrics_agent to keep packaging efforts rolling.

@vnlitvinov vnlitvinov added the automerge Merge the PR when CI passes label Jun 22, 2021
@github-actions github-actions bot merged commit 30032d2 into conda-forge:master Jun 22, 2021
@github-actions
Copy link
Contributor

Hi! This is the friendly conda-forge automerge bot!

I considered the following status checks when analyzing this PR:

  • linter: passed
  • azure: passed

Thus the PR was passing and merged! Have a great day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants