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

explorer small fix #481

Merged
merged 3 commits into from
Oct 5, 2023
Merged

explorer small fix #481

merged 3 commits into from
Oct 5, 2023

Conversation

Hakimovich99
Copy link
Contributor

use as_posix to return the absolute path. Avoids denied mounting issue when calling the explorer.

use as_posix to return the absolute path. Avoids denied mounting issue when calling the explorer.
@RobbeSneyders
Copy link
Member

Thanks @Hakimovich99! Not sure if this will work on windows though. We can ask @janvanlooyml6 or @GeorgesLorre to test.

@Hakimovich99
Copy link
Contributor Author

Thanks @Hakimovich99! Not sure if this will work on windows though. We can ask @janvanlooyml6 or @GeorgesLorre to test.

Works on Windows as well:)

@GeorgesLorre
Copy link
Collaborator

GeorgesLorre commented Oct 4, 2023

cmd.extend(
      ["-v", f"/{shlex.quote(host_machine_path)}:/{shlex.quote(container_path)}"],
)

Do we still need the slashes here ? (line 63)

@Hakimovich99
Copy link
Contributor Author

cmd.extend(
      ["-v", f"/{shlex.quote(host_machine_path)}:/{shlex.quote(container_path)}"],
)

Do we still need the slashes here ? (line 63)

Seems to work without them

cmd.extend(
>       ["-v", f"{shlex.quote(host_machine_path)}:{shlex.quote(container_path)}"],
> )

@RobbeSneyders
Copy link
Member

Can you remove the slashes as part of this PR as well?

@Hakimovich99
Copy link
Contributor Author

Can you remove the slashes as part of this PR as well?

Well can't figure out why but the checks are not successful after removing the slashes... Any idea @RobbeSneyders ?

@RobbeSneyders
Copy link
Member

You'll need to remove the slashes in the failing test as well:
https://github.com/ml6team/fondant/blob/explorer_fix/tests/test_explorer.py#L59

@Hakimovich99
Copy link
Contributor Author

You'll need to remove the slashes in the failing test as well: https://github.com/ml6team/fondant/blob/explorer_fix/tests/test_explorer.py#L59

Done, thanks Robbe!

Copy link
Member

@RobbeSneyders RobbeSneyders left a comment

Choose a reason for hiding this comment

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

Thanks!

@RobbeSneyders RobbeSneyders merged commit 76d68f8 into main Oct 5, 2023
5 checks passed
@RobbeSneyders RobbeSneyders deleted the explorer_fix branch October 5, 2023 09:03
Hakimovich99 added a commit that referenced this pull request Oct 16, 2023
use as_posix to return the absolute path. Avoids denied mounting issue
when calling the explorer.
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