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

Jump to bottom

Double slashes in metaflow.S3 keys #684

Open
tuulos opened this issue Sep 7, 2021 · 3 comments · May be fixed by #729
Open

Double slashes in metaflow.S3 keys #684

tuulos opened this issue Sep 7, 2021 · 3 comments · May be fixed by #729
Labels
good first issue

Comments

@tuulos
Copy link
Collaborator

@tuulos tuulos commented Sep 7, 2021

With a config like this

{
    "METAFLOW_DATASTORE_SYSROOT_S3": "s3://mf-test/metaflow/",
}

(note a slash after METAFLOW_DATASTORE_SYSROOT_S3)

metaflow.S3(run=self).put* produces double-slashes like here:

s3://mf-test/metaflow//data/DataLoader/1630978962283843/month=01/data.parquet

The trailing slash in the config shouldn't make a difference
@vamagithub
Copy link

@vamagithub vamagithub commented Sep 18, 2021

I would like to work on this.

@himanshu007-creator
Copy link

@himanshu007-creator himanshu007-creator commented Sep 20, 2021

With a config like this

{
    "METAFLOW_DATASTORE_SYSROOT_S3": "s3://mf-test/metaflow/",
}

(note a slash after METAFLOW_DATASTORE_SYSROOT_S3)

metaflow.S3(run=self).put* produces double-slashes like here:

s3://mf-test/metaflow//data/DataLoader/1630978962283843/month=01/data.parquet

The trailing slash in the config shouldn't make a difference

So should we remove the backslash from config!?

@romain-intel
Copy link
Contributor

@romain-intel romain-intel commented Sep 21, 2021

@vamagithub : please do submit a patch. It's probably a strip missing from somewhere.

vamagithub pushed a commit to vamagithub/metaflow that referenced this issue Sep 30, 2021
fix: Removed forward slash to prevent double slashes Netflix#684
53da6f2
@vamagithub vamagithub linked a pull request that will close this issue Sep 30, 2021
Open
vamagithub pushed a commit to vamagithub/metaflow that referenced this issue Oct 6, 2021
fix: Removed forward slash to prevent double slashes on DATATOOLS SUF…
999ad1e 
…FIX Netflix#684
vamagithub pushed a commit to vamagithub/metaflow that referenced this issue Oct 8, 2021
fix: Removing the slash from both front and back from DATATOOLS_SUFFIX …
f8ca163 
…Netflix#684
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

fix: Removed forward slash to prevent double slashes
5 participants
@tuulos @savingoyal @romain-intel @vamagithub @himanshu007-creator