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

Jump to bottom

Add SageMaker Notebook Instance module #12

Merged
merged 18 commits into from
Feb 29, 2024
Merged

Add SageMaker Notebook Instance module #12

merged 18 commits into from
Feb 29, 2024

Conversation

lccasagrande
Copy link
Contributor

@lccasagrande lccasagrande commented Feb 27, 2024 •
edited
Loading

Describe your changes

  • Added SageMaker Notebook Instance module

Checklist before requesting a review

  • I updated CHANGELOG.MD with a description of my changes
  • If the change was to a module, I ran the code validation script (scripts/validate.sh)
  • If the change was to a module, I have added thorough tests
  • If the change was to a module, I have added/updated the module's README.md
  • If a module was added, I added a reference to the module to the repository's README.md
  • I verified that my code deploys successfully using seedfarmer apply

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@kukushking
Copy link
Contributor

Hey @lccasagrande looks like the module checks failed. I should mention of that into CONTRIBUTORS.MD - we currently use global (all modules) scripts/validate.sh & scripts/fix.sh in the workflows, that are based on flake8, black, and isort.

I like ruff and I totally support moving all modules to use it but that is probably a scope of another PR... Could you use existing checks to format/validate the module code, please? And we can file another PR to replace existing checks with ruff.

Example below:
scripts/validate.sh --language python --path modules/sagemaker/sagemaker-endpoint
scripts/fix.sh --language python --path modules/sagemaker/sagemaker-endpoint

kukushking
kukushking approved these changes Feb 29, 2024
View reviewed changes
Copy link
Contributor

@kukushking kukushking left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

LeonLuttenberger
LeonLuttenberger approved these changes Feb 29, 2024
View reviewed changes
modules/sagemaker/sagemaker-notebook/setup.cfg
Comment on lines +21 to +28
[mypy]
python_version = 3.7
strict = True
ignore_missing_imports = True
allow_untyped_decorators = True
exclude =
codeseeder.out/|example/|tests/|.venv
warn_unused_ignores = False
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: This could be included in pyproject.toml. However, there's no need to block this PR on this.

modules/sagemaker/sagemaker-notebook/sagemaker_notebook/settings.py
)


class SeedFarmerParameters(CdkBaseSettings):
Copy link
Contributor

Choose a reason for hiding this comment

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

I love this!

@kukushking kukushking merged commit 0ad4754 into awslabs:main Feb 29, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeonLuttenberger LeonLuttenberger LeonLuttenberger approved these changes

@kukushking kukushking kukushking approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lccasagrande @kukushking @LeonLuttenberger