Skip to content

Fix: Catch case where no requirements argument is sent to create S… - #5527

Closed
CongaJAMM wants to merge 1 commit into
aws:masterfrom
CongaJAMM:fix/get_code_hash
Closed

CongaJAMM wants to merge 1 commit into
aws:masterfrom
CongaJAMM:fix/get_code_hash

Conversation

@CongaJAMM

Copy link
Copy Markdown

Noticed SourceCode.requirements is an optional argument and causes an error when left to default None value in a TrainingStep as part of a Pipeline.

Catches case where no requirements argument is sent to create SourceCode object and get_code_hash fails to provide a List to the get_training_code_hash function.

Issue #, if available:

Description of changes:
From the get_code_hash function created a conditional statement to get the right DataType to pass to get_training_code_hash and supply a default value.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ourceCode object and `get_code_hash` fails to provide a List to the `get_training_code_hash` function

@hillygoose hillygoose left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Elegant solution that allows the omittance of a requirements.txt file to be sent as per upstream functionality. I tried this change out as was having similar issues with this function and it worked for me

@mollyheamazon

Copy link
Copy Markdown
Contributor

Please add at least a unit test covering SourceCode(requirements=None) in a TrainingStep pipeline is needed to prevent regression.

if source_code:
source_dir = source_code.source_dir
requirements = source_code.requirements
requirements = [source_code.requirements] if source_code.requirements else []

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change to
requirements = source_code.requirements or None

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

Would you still like to merge this ? If so- please make the change asked for above.

@mohamedzeidan2021

Copy link
Copy Markdown
Collaborator

Thanks for contributing! I investigated a bit further and it seems like #5618 makes the changes that your PR attempts to fix. Closing for now, but please reopen or comment if there are any concerns.

This branch had an error being deployed

1 failed deployment
manual-approval — 1213c506 Deployed Jan 30, 2026 by CongaJAMM via wait-for-approval #228
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.

4 participants