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.

Sign up for GitHub

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

Adding friendlier exception if no S3 config provided #853

Closed
wants to merge 2 commits into from
Closed

Adding friendlier exception if no S3 config provided #853

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6aa25d6
added exception for missing s3 config
exotikh3 Dec 7, 2021
e97f425
added another check
exotikh3 Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion metaflow/datatools/s3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,12 @@ def reset_client(self):
class S3(object):
@classmethod
def get_root_from_config(cls, echo, create_on_absent=True):
return DATATOOLS_S3ROOT
if DATATOOLS_S3ROOT is None:
raise MetaflowS3URLException(
"No config or correct S3 root path provided when trying to use S3 storage"
)
else:
return DATATOOLS_S3ROOT

def __init__(
self, tmproot=".", bucket=None, prefix=None, run=None, s3root=None, **kwargs
Expand Down Expand Up @@ -316,6 +321,12 @@ def __init__(
bucket = parsed.netloc
if not prefix:
prefix = parsed.path

if DATATOOLS_S3ROOT is None:
raise MetaflowS3URLException(
"No config or correct S3 root path provided when trying to use S3 storage"
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

why only check this inside the if run.. if not prefix.. ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is only used in the if run portion so IMHO, it could be checked there. I would also check it in get_root_from_config (used for IncludeFile).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@exotikh3 Can you address this comment so that we can proceed with the merge?

if isinstance(run, FlowSpec):
if current.is_running_flow:
prefix = os.path.join(prefix, current.flow_name, current.run_id)
Expand Down