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

DOCS-#6949: Create Modin on Dask cluster tutorial #6950

Merged
merged 8 commits into from Feb 27, 2024

Conversation

Retribution98
Copy link
Collaborator

What do these changes do?

  • first commit message and PR title follow format outlined here

    NOTE: If you edit the PR title to match this format, you need to add another commit (even if it's empty) or amend your last commit for the CI job that checks the PR title to pick up the new PR title.

  • passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
  • passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
  • signed commit with git commit -s
  • Resolves Create Modin on Dask cluster tutorial #6949
  • tests added and passing
  • module layout described at docs/development/architecture.rst is up-to-date

Signed-off-by: Kirill Suvorov <kirill.suvorov@intel.com>
"cell_type": "markdown",
"metadata": {},
"source": [
"The next step is to setup your AWS credentials. One can set ``AWS_ACCESS_KEY_ID``, ``AWS_SECRET_ACCESS_KEY``\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about aws configure? Is there such an option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think no, because aws configure requires interactive mode, but this is not possible for Jupyter Notebook.

Retribution98 and others added 2 commits February 20, 2024 14:46
Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>
Comment on lines 71 to 73
"os.environ[\"AWS_ACCESS_KEY_ID\"] = \"\"\n",
"os.environ[\"AWS_SECRET_ACCESS_KEY\"] = \"\"\n",
"os.environ[\"AWS_SESSION_TOKEN\"] = \"\""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should these be empty strings? If not, Let's specify "<aws_access_key_id>", "<aws_secret_access_key>", "<aws_session_token>".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If these strings are empty, connection to the AWS will be failed.
Ok, let's specify this as you suggest.

Comment on lines 202 to 212
" try:\n",
" dir_name = os.path.dirname(file_path)\n",
" if not os.path.exists(dir_name):\n",
" os.makedirs(dir_name)\n",
" if os.path.exists(file_path): # os.path.isfile(file_path):\n",
" return \"File has already existed.\"\n",
" else:\n",
" urllib.request.urlretrieve(file_url, file_path)\n",
" return \"OK\"\n",
" except Exception as ex:\n",
" return str(ex)"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
" try:\n",
" dir_name = os.path.dirname(file_path)\n",
" if not os.path.exists(dir_name):\n",
" os.makedirs(dir_name)\n",
" if os.path.exists(file_path): # os.path.isfile(file_path):\n",
" return \"File has already existed.\"\n",
" else:\n",
" urllib.request.urlretrieve(file_url, file_path)\n",
" return \"OK\"\n",
" except Exception as ex:\n",
" return str(ex)"
" os.makedirs(filepath, exist_ok=True)
" urllib.request.urlretrieve(file_url, file_path)

Can we use something like this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can, but in this case we will have some problems:

  • if any exception is raised we won't receive any messages (Dask worker doesn't send any outputs so it must be managed by user)
  • Since this function may be called more than once. we mast guarantee the correct and predictable result.

In my opinion my suggestion is better.

Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>
Retribution98 and others added 3 commits February 26, 2024 13:57
Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>
YarShev
YarShev previously approved these changes Feb 26, 2024
@YarShev
Copy link
Collaborator

YarShev commented Feb 26, 2024

@anmyachev, any comments?

anmyachev
anmyachev previously approved these changes Feb 26, 2024
Copy link
Collaborator

@anmyachev anmyachev left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Anatoly Myachev <anatoliimyachev@mail.com>
@Retribution98 Retribution98 dismissed stale reviews from anmyachev and YarShev via 9914b3b February 27, 2024 09:46
@YarShev YarShev merged commit 271d98b into modin-project:master Feb 27, 2024
11 checks passed
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.

Create Modin on Dask cluster tutorial
3 participants