Skip to content

Conversation

@drduhe
Copy link
Collaborator

@drduhe drduhe commented Sep 17, 2025

WIP

Issue #, if available: n/a

Notes

Implement ship model endpoint for change: awslabs/osml-models#57

Changes

  • Added ship model endpoint configuration to METestEndpointsConfig
  • Added DEPLOY_SM_SHIP_ENDPOINT flag (defaults to false)
  • Added SM_SHIP_MODEL configuration (defaults to "ship")
  • Implemented ship model SageMaker endpoint creation in METestEndpoints class

Key Details

  • Ship model endpoint uses the same container image as other models
  • Configured with MODEL_SELECTION environment variable set to "ship"
  • Uses GPU instance type and VPC subnets like other model endpoints
  • Disabled by default (DEPLOY_SM_SHIP_ENDPOINT: false)

Files Modified

  • lib/osml/model_endpoint/me_test_endpoints.ts (+68 lines)

The implementation follows the same pattern as existing model endpoints (aircraft, centerpoint, flood) but is specifically configured for ship detection models.

Checklist

Before you submit a pull request, please make sure you have the following:

  • Code changes are compact and well-structured to facilitate easy review
  • Changes are documented in the README.md and other relevant documentation pages
  • PR title and description accurately reflect the changes and are detailed enough for historical tracking
  • PR contains tests that cover all new code and the code has been manual tested
  • All new dependencies are declared (if any), and no unnecessary libraries are added
  • Performance impacts (if any) of the changes are evaluated and documented
  • Security implications of the changes (if any) are reviewed and addressed
  • I have read the Contributing Guidelines and agree to follow the Code of Conduct

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

@drduhe drduhe force-pushed the feat/implement-ship-model branch from f4ca080 to 8055a38 Compare September 17, 2025 20:15
@github-actions
Copy link

github-actions bot commented Sep 17, 2025

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
91% (-0.24% 🔻)
657/722
🔴 Branches
58.59% (-0.53% 🔻)
150/256
🟢 Functions 95.38% 62/65
🟢 Lines
90.97% (-0.24% 🔻)
655/720
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / me_test_endpoints.ts
87.23% (-3.68% 🔻)
48.65% (-2.87% 🔻)
100%
87.23% (-3.68% 🔻)

Test suite run success

42 tests passing in 16 suites.

Report generated by 🧪jest coverage report action from c7979c1

}

if (this.config.DEPLOY_SM_SHIP_ENDPOINT) {
this.aircraftModelEndpoint = new MESMEndpoint(
Copy link
Contributor

Choose a reason for hiding this comment

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

concern: Looks like cut and paste errors here. Do you really want to overwrite this.aircraftModelEndpoint with the ship model endpoint or should that be a new attribute?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was an error and it's already fixed in my dev branch where I am testing stuff.

DEPLOY_SM_AIRCRAFT_ENDPOINT: true,
DEPLOY_SM_CENTERPOINT_ENDPOINT: true,
DEPLOY_SM_FLOOD_ENDPOINT: true,
DEPLOY_SM_SHIP_ENDPOINT: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

concern: It seems like the if-check on line #358 (new) #344 (old) needs to be updated to consider this flag.

Copy link
Collaborator Author

@drduhe drduhe Sep 23, 2025

Choose a reason for hiding this comment

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

There is an update to this PR coming - it's still a WIP - updated overview to state such. I.E. why I didn't check any of the boxes in the PR overview yet.

Copy link
Contributor

@edparris edparris left a comment

Choose a reason for hiding this comment

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

question: It looks to me like the issues noted would make this not work at all. Were these changes tested?

@drduhe drduhe force-pushed the feat/implement-ship-model branch from 8055a38 to c7979c1 Compare September 24, 2025 16:31
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.

2 participants