Migrate to pathlib

[Rocamonde]

Description

Fixes #531.

Migrate all path manipulation operations to be done under the pathlib package, a safe and robust way of handling paths accross platforms.

• remove os.path
• search any objects working as paths that are str and convert them to pathlib.Path
• search for osp

Design Principles

• Pass around pathlib.Path objects only (except in some special places) accross internal layers of the API
• Allow pathlib.Path, str, bytes, or path-like and convert with validation to pathlib.Path at the highest API level (user level)
• Paths in config dictionaries to remain stored as strings (for now, until someone checks what these things actually do)
• Force helper functions to receive absolute paths, construct relative paths explicitly using cwd only at the highest level that is viable.
• Force all helper functions to either create a directory or check if it exists before operating on the path

The reason for enforcing these checks is:

• [...]: For consistency. If we are going to use Path objects, we should use them everywhere that is viable, unless there is a good reason for it
• [...]: Forcing a user to pass a pathlib.Path object can be annoying and seems like an unnecessary requirement. We can have a function that normalizes string paths and makes the necessary checks.
• [...]: Not sure whether those config objects are used in other places that don't accept Path objects, and checking seems complicated, so external help would be valuable
• [...]: Functions that are not managing the working environment should not be making assumptions about where folders should be stored. It is also easier to debug IO errors if paths are absolute, as one can add a checkpoint and cd into the folder directly.
• [...] Explicitly checking if a folder exists or creating is self-documenting of the role of the function.

Testing

Current tests should suffice, if the migration is done properly nothing should break. No new features are being added.

• Add tests where it is viable to check whether helper functions using paths are disallowing relative paths

[Rocamonde]

imitation.scripts.common.demonstrations:guess_expert_dir() is trying to assert that the data dir is absolute, but this is not the case (it's always passed a relative dir). When looking up where this path is constructed and whether the dir is relative to the current working directory, I found something weird.

The dagger object passed below:

imitation/src/imitation/scripts/train_imitation.py

Lines 99 to 108 in 81d1594

	@train_imitation_ex.capture
	def train_imitation(
	_run,
	bc_kwargs: Mapping[str, Any],
	bc_train_kwargs: Mapping[str, Any],
	dagger: Mapping[str, Any],
	use_dagger: bool,
	agent_path: Optional[str],
	) -> Mapping[str, Mapping[str, float]]:
	"""Runs DAgger (if `use_dagger`) or BC (otherwise) training.

contains an expert_policy_path key that points to data/expert_models/cartpole_0/policies/final, but this path never exists during the lifecycle of the tests (not relative to the cwd or to any other directory), nor is it used anywhere in the tests. The actual location of the policy is in tests/testadata/. This is injected through the TEST_DATA_PATH when the policy is actually accessed, but this strange path is still getting constructed.

1. It seems valuable to ensure that paths are absolute wherever possible, as it allows spotting errors like these.
2. I could force the path to be constructed relative to the cwd somewhere, so that the assertion passes, but this seems strange because I don't actually know what this path would be for or if it would be used in some circumstance/setting.

[Rocamonde]

Something closely related to this is what happens below:

imitation/src/imitation/scripts/common/demonstrations.py

Lines 37 to 49 in 81d1594

	@demonstrations_ingredient.config_hook
	def hook(config, command_name, logger):
	"""If rollout_path not set explicitly, then guess it based on environment name."""
	del command_name, logger
	updates: Dict[str, str] = {}
	if config["demonstrations"]["rollout_path"] is None:
	data_dir = config["demonstrations"]["data_dir"]
	env_name = config["common"]["env_name"].replace("/", "_")
	updates["rollout_path"] = str(
	guess_expert_dir(data_dir, env_name) / "rollouts" / "final.pkl"
	)
	return updates

A similar path is constructed when config["demonstrations"]["rollout_path"] is None, but this never happens because this is where the correct path gets injected from the TEST_DATA_PATH.

I presume this is some sort of fallback-setting feature that tries to guess where the files are, but cannot find the test corresponding to this feature.

[AdamGleave]

Thanks for making this change, seems like a significant improvement to our path handling.

Most of my comments are nits. The one substantive one is to actually add some tests for types.parse_path -- shouldn't be too hard.

[AdamGleave]

LGTM, some small suggestions