Create local artifact directory if it does not exist #847
Conversation
| def is_remote_path(path: Path) -> bool: | ||
| """Check if the path is remote.""" | ||
| _path = str(path) | ||
| prefixes = set(known_implementations.keys()) - {"local", "file"} | ||
| return any(_path.startswith(prefix) for prefix in prefixes) |
There was a problem hiding this comment.
Wouldn't it be easier to swap this check and check if it's local instead?
There was a problem hiding this comment.
Not sure. The remote prefixes are well defined by the known_implementations. The local paths can be more complex (start with prefix, without prefix, absolute path, relative path, ...). Thought a check for a remote path is easier.
| @@ -270,7 +270,7 @@ def test_local_run(mock_docker_installation): | |||
| """Test that the run command works with different arguments.""" | |||
| args = argparse.Namespace( | |||
| local=True, | |||
| ref="some/path", | |||
| ref=__name__, | |||
There was a problem hiding this comment.
Why are we using __name__ here?
There was a problem hiding this comment.
I don't understand completely why the tests have worked before. The actual module contains a test pipeline which is used as reference for the test cases. __name__ makes more sense here.
However, I would cleanup the test cases as well. I think we can extract some fixtures here and make it more robust.
| @@ -168,6 +169,7 @@ def test_docker_compiler(setup_pipeline, tmp_path_factory): | |||
| example_dir, pipeline, _ = setup_pipeline | |||
| compiler = DockerCompiler() | |||
| with tmp_path_factory.mktemp("temp") as fn: | |||
| pipeline.base_path = str(fn) | |||
There was a problem hiding this comment.
Not a big fan of this. Can we turn the fixture into a factory function?
There was a problem hiding this comment.
If it's fine for you I would exclude it here and keep the test as it is. I'll create an separat issue for this. I took a look into the test and I think it would result in a bigger refactoring. It would make indeed sense to have a pipeline factory cause it is needed in several test cases. Not only here in this file.
There was a problem hiding this comment.
We already have code like this that we could reuse more.
eg:
fondant/tests/pipeline/test_compiler.py
Line 132 in 7ec04dc
This is one is even parameterized so you can easily run the same test on multiple pipelines.
This bothers me already for some time. Change the mounting of the local base path in the docker compile. Now a local base path is created.