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

Propagate self.env if it was set. #916

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

rohinb2
Copy link
Contributor

@rohinb2 rohinb2 commented Jun 18, 2024

Breaking example, where we initialize reqs in the constructor, which gets set in the env, but is not propagated in .to because the env has no name. Pretty sure we can just send self.env and later we have a check if it has no name to take a different path.

See test for repro.

Copy link

sentry-io bot commented Jun 19, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: runhouse/resources/module.py

Function Unhandled Issue
to [**ValueError: Need an initialized local server in order to put a module onto rh.here. Please run runhouse re...**](https://runhouse.sentry.io/issues/5259949416/?referrer=github-open-pr-bot) ... <br> Event Count:` 1

Did you find this useful? React with a 👍 or 👎

Copy link
Contributor Author

rohinb2 commented Jun 19, 2024

Copy link
Collaborator

@carolineechen carolineechen left a comment

Choose a reason for hiding this comment

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

minor comment on the test but core logic looks good

@@ -30,6 +30,12 @@ def np_summer(a, b):
return int(np.sum([a, b]))


def torch_import():
import torch
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we use a diff library that's not used in any other test env/function yet to make sure its standalone? aws_cpu fixture now uses a torch docker image

Copy link
Contributor Author

rohinb2 commented Jun 20, 2024

Merge activity

  • Jun 20, 2:47 PM EDT: @rohinb2 started a stack merge that includes this pull request via Graphite.
  • Jun 20, 2:48 PM EDT: Graphite rebased this pull request as part of a merge.
  • Jun 20, 2:49 PM EDT: @rohinb2 merged this pull request with Graphite.

@rohinb2 rohinb2 force-pushed the 06-18-Propagate_self.env_if_it_was_set branch from a83956c to bc446aa Compare June 20, 2024 18:48
@rohinb2 rohinb2 merged commit 30bd6c4 into main Jun 20, 2024
12 checks passed
@rohinb2 rohinb2 deleted the 06-18-Propagate_self.env_if_it_was_set branch June 20, 2024 18:49
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.

3 participants