Skip to content
This repository has been archived by the owner on Nov 29, 2024. It is now read-only.

Dynamically set capabilities endpoint based on configuration #281

Merged
merged 4 commits into from
Jan 11, 2022

Conversation

nkpng2k
Copy link
Contributor

@nkpng2k nkpng2k commented Jan 5, 2022

Small improvement to /model/capabilities endpoint. No longer hardcoded. Pulls configuration from the scorer and uses that to set the response.

@nkpng2k nkpng2k requested a review from orendain January 5, 2022 20:19
@nkpng2k
Copy link
Contributor Author

nkpng2k commented Jan 6, 2022

resolves #279

Copy link
Member

@orendain orendain left a comment

Choose a reason for hiding this comment

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

LGTM - awesome!

@nkpng2k nkpng2k requested a review from orendain January 11, 2022 01:36
Copy link
Member

@orendain orendain left a comment

Choose a reason for hiding this comment

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

LGTM - tests seem valid 👍🏾. Added some notes to help curb the workarounds or improve maintainability.

local-rest-scorer/build.gradle Outdated Show resolved Hide resolved

private static void mockDummyPipeline() {
MojoPipeline dummyPipeline =
new DummyPipeline(TEST_UUID, MojoFrameMeta.getEmpty(), MojoFrameMeta.getEmpty());
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if our static mock troubles would be fixed if we moved the loadMojoPipelineFromFile() call in MojoScorer.java from static to the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe... Seems like a bigger rework than I'm inclined to tackle at the moment.

@BeforeAll
static void setup() {
System.setProperty("mojo.path", "src/test/resources/multinomial-pipeline.mojo");
mockDummyPipeline();
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't look like the static mock session was closed. Not a big deal since the class is small and each test relies on this.

But for thoroughness, I'll share a couple of ways through this:

  1. Perform staticmock as part of a try() at the place it's needed, which auto cleans up the mock after use. Example.
  2. Save the mock to a static variable and close the session as part of a @AfterAll.

@nkpng2k
Copy link
Contributor Author

nkpng2k commented Jan 11, 2022

LGTM - tests seem valid 👍🏾. Added some notes to help curb the workarounds or improve maintainability.

Thanks will fix and merge I think.

@nkpng2k nkpng2k merged commit 8ee42a4 into master Jan 11, 2022
@nkpng2k nkpng2k deleted the npng/master/dynamically-set-capabilities-endpoint branch January 11, 2022 21:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants