Update _get_env_from to handle cases better

[carolineechen]

improve 2 cases of _get_env_from, which is expected to return an Env type:

1. input env = "base_env" -- falls through all the if statements and returns the string "base_env" instead of the Env object
2. env is not saved in rns_client but exists on the cluster associated with the call. returns the env string instead of Env object

[carolineechen]

• Update _get_env_from to handle cases better #398 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @carolineechen and the rest of your teammates on Graphite

[carolineechen]

@dongreenberg can you double check my logic here and if this PR makes sense? and is there an instance where it still makes sense to return the input env if it doesn't satisfy any of the conditions or to simply throw an error?

most cases we expect an Env to be returned which will throw an error elsewhere if it returns a string and we try to call an env property on it. but there could also be cases where the output is simply used to populate a config etc, not sure if it's an issue if a string is returned if the corresponding Env could not be found.

[dongreenberg]

tbh I'm nervous that passing system here is a little non-deterministic. Den and this system could have different configs for that env name, and the only thing determining which is returned is the order we've implemented here. What case are we solving for where we want to check on the cluster for the env config?

I also don't think I'd raise a warning here. We use this method all the time in instances where we don't necessarily expect and env to be found (but should construct one if it is), and it'll be pretty noisy for the user.

[dongreenberg]

Tbh I think I'm missing some context, but there's some back and forth here with the cluster that seems a bit hairy to me. Let's chat about it live

[dongreenberg]

Suggested change

	if isinstance(env, Resource) or env is None:
	return env
	from runhouse.resources.envs import Env
	from runhouse.resources.envs import Env
	if isinstance(env, Env) or env is None:
	return env

[dongreenberg]

Suggested change

	if system.get(env):
	return system.get(env)
	# handle case where env is passed in as full rns address
	name, _ = rns_client.split_rns_name_and_path(
	rns_client.resolve_rns_path(env)
	)
	if system.get(name):
	return system.get(name)
	# handle case where env is passed in as full rns address
	name, _ = rns_client.split_rns_name_and_path(
	rns_client.resolve_rns_path(env)
	) if "/" in env else (env, None)
	if system.get(name):
	return system.get(name)

[dongreenberg]

Just saves a get

[dongreenberg]

tbh I'm nervous that passing system here is a little non-deterministic. Den and this system could have different configs for that env name, and the only thing determining which is returned is the order we've implemented here. What case are we solving for where we want to check on the cluster for the env config?

I also don't think I'd raise a warning here. We use this method all the time in instances where we don't necessarily expect and env to be found (but should construct one if it is), and it'll be pretty noisy for the user.

[dongreenberg]

This makes me a bit nervous - if the env contains package objects they will be serialized and sent back pointing to the cluster's local filesystem. Also not totally clear what's happening here, we're getting the env object from the system by name so we can then extract the name out of it?

[dongreenberg]

I also think perhaps the base_env and base default naming business is confusing me here. We should probably rename those or eliminate the overlap asap.