-
Notifications
You must be signed in to change notification settings - Fork 14
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
support for caliper graded profile #759
Merged
Merged
Changes from 4 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
37942b0
support for caliper graded profile
jreddig be6677b
add iso duration
jreddig b099be9
remove unused imports
jreddig 17bd0a6
export interfaces
jreddig 3e012c2
add caliper endpoint to v1 api
jreddig 782546c
Revert "add iso duration"
jreddig 214a9ec
stricter typing
jreddig 352d450
add caliper actor
jreddig 93ddb32
error handling for unsupported profiles and events
jreddig 7798f5d
update caliper data object
jreddig cbdd033
add to v1
jreddig 9516f10
add caliper enums to interfaces
jreddig b9c4080
Merge branch 'dev' of https://github.com/CarnegieLearningWeb/UpGrade …
jreddig a760dce
resolve conflicts
jreddig 527371e
add duration package
jreddig 34f3e7e
match log file
jreddig ced85c6
update version
jreddig c5606ae
Merge branch 'dev' into caliper-graded-profile
jreddig bc289ae
use upgrade types
jreddig db09e25
Merge branch 'dev' of https://github.com/CarnegieLearningWeb/UpGrade …
jreddig a9c9a13
resolve conflicts
jreddig 9ed1605
Merge branch 'dev' of https://github.com/CarnegieLearningWeb/UpGrade …
jreddig ef10d8e
update lockfile
jreddig 13e852c
Merge branch 'dev' into caliper-graded-profile
jreddig File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
39 changes: 39 additions & 0 deletions
39
backend/packages/Upgrade/src/api/controllers/validators/CaliperLogData.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import { IsNotEmpty, IsDefined, IsString, IsJSON, IsObject } from 'class-validator'; | ||
import { Attempt, CaliperActor, ScoreObject } from 'upgrade_types'; | ||
|
||
export class CaliperLogData { | ||
|
||
@IsDefined() | ||
@IsNotEmpty() | ||
@IsString() | ||
public profile: string; | ||
|
||
@IsDefined() | ||
@IsNotEmpty() | ||
@IsJSON() | ||
public actor: CaliperActor; | ||
|
||
@IsDefined() | ||
@IsNotEmpty() | ||
@IsString() | ||
public action: string; | ||
|
||
@IsDefined() | ||
@IsNotEmpty() | ||
@IsString() | ||
public eventTime: string; | ||
|
||
@IsDefined() | ||
@IsNotEmpty() | ||
@IsJSON() | ||
public object: Attempt; | ||
|
||
@IsObject() | ||
public extensions: object; | ||
|
||
@IsObject() | ||
@IsNotEmpty() | ||
@IsJSON() | ||
public generated: ScoreObject; | ||
|
||
} |
22 changes: 22 additions & 0 deletions
22
backend/packages/Upgrade/src/api/controllers/validators/CaliperLogEnvelope.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { IsNotEmpty, IsDefined, IsString } from 'class-validator'; | ||
import { CaliperLogData } from './CaliperLogData'; | ||
|
||
|
||
export class CaliperLogEnvelope { | ||
@IsDefined() | ||
@IsNotEmpty() | ||
@IsString() | ||
public sensor: string; | ||
|
||
@IsDefined() | ||
@IsNotEmpty() | ||
@IsString() | ||
public sendTime: string; | ||
|
||
@IsDefined() | ||
@IsNotEmpty() | ||
@IsString() | ||
public dataVersion: string; | ||
|
||
public data: CaliperLogData[]; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import { Types, Interfaces } from '../identifiers'; | ||
import fetchDataService from '../common/fetchDataService'; | ||
import { CaliperEnvelope } from '../../../../types/src/Experiment/interfaces'; | ||
|
||
export default async function logCaliper( | ||
url: string, | ||
userId: string, | ||
token: string, | ||
clientSessionId: string, | ||
value: CaliperEnvelope, | ||
sendAsAnalytics = false | ||
): Promise<Interfaces.ILog[]> { | ||
try { | ||
const logResponse = await fetchDataService( | ||
url, | ||
token, | ||
clientSessionId, | ||
value, | ||
Types.REQUEST_TYPES.POST, | ||
sendAsAnalytics | ||
); | ||
if (logResponse.status) { | ||
return logResponse.data; | ||
} else { | ||
throw new Error(JSON.stringify(logResponse.message)); | ||
} | ||
} catch (error) { | ||
throw new Error(error.message); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object
andObject
Typescript types are less useful than they appear, as it will allow anything that is an object at all (arrays are objects!) and if you were to do writeextensions.something
it will complain thatsomething
is an unknown property ofextensions
.There are a couple of alternatives that I see floated around to avoid
object
type where all you really know is that you could get an object with random stuff, which is either:{ [key: string] : any }
orRecord<string, any>
I also see
Record<string, unknown>
, but I'm not 100% the benefit of unknown vs any. Not sure how you'd get around any or unknown thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The end goal is to use the attempt extensions for additional metrics, so I could make it a ILogInput type? Is it too restricting to say that you can only include our metric format as additional data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
Record<string, unknown>
is what we want if we are just throwing out this data and/or not doing any processing of those vars to try to access data on them or transform before writing to db.Based on my thorough stack overflow skimming,
any
is like turning off type-checking entirely, whereunknown
means allow anything, but yell at me if I actually try to use it for anything.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe the type can be
ILogInput | Record<string, unknown>
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe not; I think you'd just want
Record<string, unknown>
if we don't want to force the format... I think the use case would be to flexibly allow any unknown object, but yell at anyone who tries to access properties onextensions
to remind them that nothing is guaranteed about these objects.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like
Record<string, unknown>
to just pass it on and save the metrics. If I do the same on the client lib side, does unknown make it hard to create the object?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question... I guess it could. I think it's probably ok to say
Record<string, any>
if we're going to allow any objects, as they'd run into annoying issues if they were creating the object in steps:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how we're providing proper types actually though, or if anyone is using types for interacting with the library currently. If it's just being used as javascript, or if we aren't providing these types, none of it really matters in the client side.