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.

Sign up for GitHub

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

Jump to bottom

ML: Add write type and have scripts return writes instead of calling bridge #119

Merged
merged 10 commits into from
May 9, 2024
Merged

ML: Add write type and have scripts return writes instead of calling bridge #119

merged 10 commits into from
May 9, 2024

Conversation

siddharth-krishna
Copy link
Collaborator

The tests work now, but we can hold off on merging this until we've checked that these changes are compatible with the orchestrator.

ngua added 4 commits May 9, 2024 11:00
@ngua
Chunk output stream; add valuesBetween
380bc61 
commit f6ceb8f
Author: Rory Tyler Hayford <[email protected]>
Date:   Tue May 7 16:56:02 2024 +0700

    Remove bridge indirection

commit e11c056
Author: Rory Tyler Hayford <[email protected]>
Date:   Tue May 7 16:26:16 2024 +0700

    Fix dummy server

commit cf8511b
Author: Rory Tyler Hayford <[email protected]>
Date:   Tue May 7 16:20:55 2024 +0700

    Fix argument order for `valuesBetween`

commit 69227fd
Author: Rory Tyler Hayford <[email protected]>
Date:   Tue May 7 16:01:43 2024 +0700

    Add implicit cast, change tuple parsing

commit b38ff79
Author: Rory Tyler Hayford <[email protected]>
Date:   Tue May 7 15:10:30 2024 +0700

    Add `valuesBetween`

commit cf504f9
Author: Rory Tyler Hayford <[email protected]>
Date:   Tue May 7 12:53:32 2024 +0700

    Update doc

commit 3fbb858
Author: Rory Tyler Hayford <[email protected]>
Date:   Tue May 7 11:29:37 2024 +0700

    Actually provide an error message

commit ea28d29
Author: Rory Tyler Hayford <[email protected]>
Date:   Mon May 6 17:33:36 2024 +0700

    Try chunking output stream

commit a638425
Merge: bf8111d bb24f11
Author: Rory Tyler Hayford <[email protected]>
Date:   Mon May 6 11:43:34 2024 +0700

    Merge branch 'main' into rory-ml-writes

commit bf8111d
Author: Rory Tyler Hayford <[email protected]>
Date:   Fri May 3 15:53:33 2024 +0700

    Compare results directly in test

commit 80279dd
Author: Rory Tyler Hayford <[email protected]>
Date:   Fri May 3 12:51:14 2024 +0700

    Make JSON encoding more compact

commit 805175b
Author: Rory Tyler Hayford <[email protected]>
Date:   Fri May 3 12:36:40 2024 +0700

    Fix imports

commit 4c8d7a9
Author: Rory Tyler Hayford <[email protected]>
Date:   Fri May 3 12:36:03 2024 +0700

    Move `makeWriteFun`

commit d224f2f
Author: Rory Tyler Hayford <[email protected]>
Date:   Fri May 3 12:03:23 2024 +0700

    Flatten list within conduit stream

commit bb24f11
Author: Rory Tyler Hayford <[email protected]>
Date:   Tue Apr 30 14:02:08 2024 +0700

    [inferno-ml] Update DB tables (#118)

    I noticed that the `params` table still references `models`, not
    `mversions`, so that needs to be fixed. Also I removed some tables that
    should be put elsewhere
@ngua
Merge branch 'main' into sidk-ml-writes
d6d7fe8
@ngua
Fix formatting
8fa2667
@ngua
Fix exe
a6d4b6c
ngua
ngua approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@ngua ngua left a comment

Choose a reason for hiding this comment

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

This seems to be working well, good performance too, so I think we can merge this now. We can potentially revisit the default chunk size in the output stream later, but at the moment it seems good to me

@ngua ngua merged commit 89470e8 into main May 9, 2024
1 check passed
@ngua ngua deleted the sidk-ml-writes branch May 9, 2024 04:29
siddharth-krishna
siddharth-krishna commented May 9, 2024
View reviewed changes
Copy link
Collaborator Author

@siddharth-krishna siddharth-krishna left a comment

Choose a reason for hiding this comment

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

Thanks Rory, I had put a bunch of hacks in to get the integration tests running quickly, and your changes make it all nicer and neater. :)

inferno-ml-server-types/src/Inferno/ML/Server/Types.hs
@@ -703,6 +704,7 @@ data IValue
| ITuple (IValue, IValue)
| ITime EpochTime
| IEmpty
| IArray (Vector IValue)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is IValue the type of all values that can be stored in input/output PIDs in TachDB? I don't think they support arrays, if so.

inferno-ml-server-types/src/Inferno/ML/Server/Types.hs
-- We don't want to confuse a two-element array of tuples with
-- a tuple itself. For example, `"[[10.0, {\"time\": 10}], [10.0, {\"time\": 10}]]"`
-- should parse as a two-element array of `(double, time)` tuples,
-- not as a `((double, time), (double, time))`. I can't think of
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha yeah I see the issue, and agree with your solution, but btw I think inferno allows n-tuples for any n != 1. As with my previous comment, I don't think Tach supports tuples, so they're only script-internal values and we wouldn't need to support them in the interface to Tach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngua ngua ngua approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@siddharth-krishna @ngua