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

chore!: rename algo to function #139

Merged
merged 71 commits into from
Feb 10, 2023
Merged

chore!: rename algo to function #139

merged 71 commits into from
Feb 10, 2023

Conversation

@ThibaultFy ThibaultFy changed the title rename algo to function chore: rename algo to function Jan 31, 2023
WHERE kind = 'ASSET_ALGO';

UPDATE events
SET asset = asset - 'algo' || JSONB_BUILD_OBJECT('function', asset->'algo')
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure what was the field name before, the test failing seems to indicate that it was algorithm and not algo

Copy link
Member Author

Choose a reason for hiding this comment

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

It is on the migration 000029, it seems to be algo:/

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
@ThibaultFy ThibaultFy marked this pull request as ready for review February 9, 2023 10:22
@ThibaultFy ThibaultFy requested a review from a team as a code owner February 9, 2023 10:22
Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
ComputeTask }o--|| Algo : execute
Algo {}
ComputeTask }o--|| Function : execute
Function {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this line?

@@ -78,7 +78,7 @@ type TestClient struct {
ks *KeyStore
logger zerolog.Logger
organizationService asset.OrganizationServiceClient
algoService asset.AlgoServiceClient
functionService asset.FunctionServiceClient
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please align the right column with the other ones?

Suggested change
functionService asset.FunctionServiceClient
functionService asset.FunctionServiceClient

@@ -128,7 +128,7 @@ func (f *TestClientFactory) NewTestClient() *TestClient {
ks: NewKeyStore(),
logger: logger,
organizationService: asset.NewOrganizationServiceClient(f.conn),
algoService: asset.NewAlgoServiceClient(f.conn),
functionService: asset.NewFunctionServiceClient(f.conn),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same alignment here

Suggested change
functionService: asset.NewFunctionServiceClient(f.conn),
functionService: asset.NewFunctionServiceClient(f.conn),

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment

Choose a reason for hiding this comment

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

Thanks for this great job! I would just like to have go fmt on the full codebase (https://go.dev/blog/gofmt) and a small change on the migration

@@ -35,7 +35,7 @@ type TaskInputOptions struct {

type TrainTaskOptions struct {
KeyRef string
AlgoRef string
FunctionRef string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FunctionRef string
FunctionRef string

@@ -45,7 +45,7 @@ type TrainTaskOptions struct {

type TestTaskOptions struct {
KeyRef string
AlgoRef string
FunctionRef string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FunctionRef string
FunctionRef string

@@ -55,7 +55,7 @@ type TestTaskOptions struct {

type PredictTaskOptions struct {
KeyRef string
AlgoRef string
FunctionRef string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FunctionRef string
FunctionRef string

@@ -65,7 +65,7 @@ type PredictTaskOptions struct {

type CompositeTaskOptions struct {
KeyRef string
AlgoRef string
FunctionRef string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FunctionRef string
FunctionRef string

Comment on lines 77 to 81
KeyRef string
AlgoRef string
FunctionRef string
PlanRef string
Worker string
Inputs []*TaskInputOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, align all the string to the furthest on the right

@@ -152,7 +152,7 @@ func (o *TestTaskOptions) GetNewTask(ks *KeyStore) *asset.NewComputeTask {
func DefaultTrainTaskOptions() *TrainTaskOptions {
return &TrainTaskOptions{
KeyRef: DefaultTrainTaskRef,
AlgoRef: DefaultSimpleAlgoRef,
FunctionRef: DefaultSimpleFunctionRef,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FunctionRef: DefaultSimpleFunctionRef,
FunctionRef: DefaultSimpleFunctionRef,

@@ -204,7 +204,7 @@ func (o *TrainTaskOptions) SetOutputs(outputs map[string]*asset.NewComputeTaskOu
func (o *TrainTaskOptions) GetNewTask(ks *KeyStore) *asset.NewComputeTask {
return &asset.NewComputeTask{
Key: ks.GetKey(o.KeyRef),
AlgoKey: ks.GetKey(o.AlgoRef),
FunctionKey: ks.GetKey(o.FunctionRef),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FunctionKey: ks.GetKey(o.FunctionRef),
FunctionKey: ks.GetKey(o.FunctionRef),

@@ -214,7 +214,7 @@ func (o *TrainTaskOptions) GetNewTask(ks *KeyStore) *asset.NewComputeTask {
func DefaultPredictTaskOptions() *PredictTaskOptions {
return &PredictTaskOptions{
KeyRef: DefaultPredictTaskRef,
AlgoRef: DefaultPredictAlgoRef,
FunctionRef: DefaultPredictFunctionRef,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FunctionRef: DefaultPredictFunctionRef,
FunctionRef: DefaultPredictFunctionRef,

@@ -252,7 +252,7 @@ func (o *PredictTaskOptions) WithDataSampleRef(ref string) *PredictTaskOptions {
func (o *PredictTaskOptions) GetNewTask(ks *KeyStore) *asset.NewComputeTask {
return &asset.NewComputeTask{
Key: ks.GetKey(o.KeyRef),
AlgoKey: ks.GetKey(o.AlgoRef),
FunctionKey: ks.GetKey(o.FunctionRef),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
FunctionKey: ks.GetKey(o.FunctionRef),
FunctionKey: ks.GetKey(o.FunctionRef),

Comment on lines 1 to 65
ALTER TABLE algos
RENAME CONSTRAINT algos_owner_channel_fkey TO functions_owner_channel_fkey;

ALTER TABLE algos
RENAME COLUMN algorithm TO functionAddress;

ALTER TABLE algos
RENAME TO functions;

ALTER TABLE compute_tasks
RENAME COLUMN algo_key TO function_key;

ALTER TABLE algo_outputs
RENAME COLUMN algo_key TO function_key;

ALTER TABLE algo_outputs
RENAME TO function_outputs;

ALTER TABLE algo_inputs
RENAME COLUMN algo_key TO function_key;

ALTER TABLE algo_inputs
RENAME TO function_inputs;

ALTER TABLE performances
RENAME COLUMN algo_key TO function_key;

ALTER TABLE performances
RENAME CONSTRAINT performances_algo_key_fkey TO performances_function_key_fkey;

DROP VIEW IF EXISTS expanded_algos;
CREATE VIEW expanded_functions AS
SELECT key,
name,
description AS description_address,
desc_add.checksum AS description_checksum,
functionAddress AS function_address,
function_add.checksum AS function_checksum,
permissions,
owner,
creation_date,
metadata,
channel
FROM functions
JOIN addressables desc_add ON functions.description = desc_add.storage_address
JOIN addressables function_add ON functions.functionAddress = function_add.storage_address;

INSERT INTO asset_kinds(kind)
VALUES ('ASSET_FUNCTION');

UPDATE events e
SET asset_kind = 'ASSET_FUNCTION'
WHERE e.asset_kind = 'ASSET_ALGO';

DELETE FROM asset_kinds
WHERE kind = 'ASSET_ALGO';

UPDATE events
SET asset = jsonb_set(asset, '{functionKey}', asset->'algoKey') - 'algoKey'
WHERE asset_kind = 'ASSET_COMPUTE_TASK' AND NOT(asset ? 'functionKey');

UPDATE events
SET asset = JSONB_SET(asset, '{function}',
asset -> 'algorithm') - 'algorithm'
WHERE asset_kind = 'ASSET_FUNCTION';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a condition around this full migration to avoid to rerun it from a dirty state?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes I wanted to do that... How do we choose the right condition ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mh ... That's a tricky one, we don't have guideliens on it but as everything should be executed in the same transaction, any of the changes can be used, for instance table_exists('public', 'functions')

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean table_exists('public', 'algos') ?

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
RENAME CONSTRAINT algos_owner_channel_fkey TO functions_owner_channel_fkey;

ALTER TABLE algos
RENAME COLUMN algorithm TO functionAddress;
Copy link
Contributor

@guilhem-barthes guilhem-barthes Feb 10, 2023

Choose a reason for hiding this comment

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

Shouldn't it be function_address?

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember having an error using "_" in the name

name,
description AS description_address,
desc_add.checksum AS description_checksum,
functionAddress AS function_address,
Copy link
Contributor

@guilhem-barthes guilhem-barthes Feb 10, 2023

Choose a reason for hiding this comment

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

shouldn't it be function_address ? (also, can you remove one space to align the one on the right .__)

desc_add.checksum AS description_checksum,
functionAddress AS function_address,
function_add.checksum AS function_checksum,
permissions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
permissions,
permissions,

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>
Copy link
Contributor

@guilhem-barthes guilhem-barthes left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the work 🙏

@ThibaultFy ThibaultFy merged commit 3151513 into main Feb 10, 2023
@ThibaultFy ThibaultFy deleted the rename-algo-to-function branch February 10, 2023 16:38
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