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

feat: otter sdk training - model extension #2411

Open
wants to merge 1 commit into
base: feat/otter-training
Choose a base branch
from

Conversation

sdo-1A
Copy link
Contributor

@sdo-1A sdo-1A commented Nov 6, 2024

Proposed change

Otter SDK Training - Model extension

NOTE: the exercise and solution files are combined with the existing training-sdk

Related issues

- No issue associated -

@sdo-1A sdo-1A requested a review from a team as a code owner November 6, 2024 09:55
Copy link

nx-cloud bot commented Nov 6, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 5ab818f. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@github-actions github-actions bot added enhancement New feature or request project:@o3r/showcase labels Nov 6, 2024
@sdo-1A sdo-1A force-pushed the feat/training-sdk-model-extension branch from 79fbfac to 5ab818f Compare November 6, 2024 09:57
const reviver = <T extends MyModel = MyModel>(data: any, dictionaries?: any) => {
const revivedData = baseRevive<MyModelCoreIfy<T>>(data, dictionaries);
/* Set the value of your new fields here */
// EXAMPLE: revivedData.myNewField = 'fake value';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Example should come from the instructions and not just something to uncomment

Ensure that the global property <code>allowModelExtension</code> has been set to <b>true</b>. This will guarantee that the revivers of the
base models are generated, which is essential for the following exercise.
</p>

Copy link
Contributor

Choose a reason for hiding this comment

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

It is a bit weird - we say that they should run commands as it is essential for the following exercise but we do it for them.
Maybe, we should be clearer:

You will be working with an sdk which has been generated with the .... option etc.

This way we make it clearer that they will need to do it if they want a local run or use it in their project

const revivedData = baseRevive<FlightCoreIfy<T>>(data, dictionaries);
/* Set the value of your new fields here */
revivedData.id = 'sampleIdValue';
return revivedData;
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be nice to show the id somewhere in a ui?

Copy link
Contributor

Choose a reason for hiding this comment

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

But they won't get the autocompletion :/ - at least the compiler should complain

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants