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

job priority #1466

Merged
merged 35 commits into from
Dec 27, 2021
Merged

job priority #1466

merged 35 commits into from
Dec 27, 2021

Conversation

golanha
Copy link
Member

@golanha golanha commented Dec 2, 2021

This change is Reviewable

golanha and others added 6 commits December 2, 2021 16:05
* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR
* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR .... bump version [skip ci]
* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR .... bump version [skip ci]
@golanha golanha requested a review from nassiharel December 5, 2021 14:06
@golanha
Copy link
Member Author

golanha commented Dec 5, 2021

/deploy

@golanha
Copy link
Member Author

golanha commented Dec 5, 2021

/deploy

@golanha
Copy link
Member Author

golanha commented Dec 6, 2021

/deploy

@hkube-ci hkube-ci temporarily deployed to dev December 6, 2021 12:58 Inactive
@golanha
Copy link
Member Author

golanha commented Dec 15, 2021

/deploy

@hkube-ci hkube-ci temporarily deployed to dev December 15, 2021 15:04 Inactive
@golanha
Copy link
Member Author

golanha commented Dec 15, 2021

/deploy

Copy link
Member Author

@golanha golanha left a comment

Choose a reason for hiding this comment

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

Reviewable status: 80 of 103 files reviewed, 9 unresolved discussions (waiting on @nassiharel and @yehiyam)


core/pipeline-driver-queue/lib/service/preferred-jobs.js, line 10 at r14 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

no async

Done.

Code quote:

 async deletePreferredJobs

core/pipeline-driver-queue/lib/service/preferred-jobs.js, line 44 at r14 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

This code is a little cumbersome.
Try to split to functions and add some comments

Done.


core/pipeline-driver-queue/lib/validation/preference.js, line 20 at r14 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

what if we will add two more variables?

if ((tag && jobId) || (tag && pipeline) || (pipeline && jobId)
|| (pipeline && jobId)|| (pipeline && jobId)|| (pipeline && jobId)
|| (pipeline && jobId)|| (pipeline && jobId)|| (pipeline && jobId)|| (pipeline && jobId)
) {

Done.

@golanha golanha requested a review from nassiharel December 21, 2021 11:52
Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 14 files at r10, 5 of 6 files at r13, 2 of 2 files at r14, 5 of 10 files at r15, all commit messages.
Reviewable status: 94 of 103 files reviewed, 3 unresolved discussions (waiting on @yehiyam)

@golanha golanha requested a review from nassiharel December 22, 2021 09:15
const routes = () => {
const router = RestServer.router();

router.get('/', async (req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not async

const response = preferredService.getPreferredJobsList();
res.json(response);
});
router.post('/', async (req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not async

const response = preferredService.getPreferredJobsList();
res.json(response);
});
router.post('/', async (req, res) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a best practice, it is confusing...
The user can set the same jobs ids in addedJobs and removedJobs...
I think you need to add router.delete with a body.

poweredBy: 'HKube Pipeline Driver queue',
bodySizeLimit: process.env.BODY_SIZE_LIMIT || '50mb'
};
config.swagger = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you need this?

preferenceRequest
);
const { position, query } = preferenceRequest.addedJobs;
if ((position === 'first' || position === 'last')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if ((position === 'first' || position === 'last') && query) {

core/pipeline-driver-queue/package.json Show resolved Hide resolved
Copy link
Member Author

@golanha golanha left a comment

Choose a reason for hiding this comment

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

Reviewable status: 87 of 104 files reviewed, 9 unresolved discussions (waiting on @nassiharel and @yehiyam)


core/pipeline-driver-queue/package.json, line 32 at r17 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

Do you need performance-now?

I didn't add it. It is currently used in scoreing.js


core/pipeline-driver-queue/config/main/config.base.js, line 18 at r17 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

Why you need this?

You are right, I didn't use it.


core/pipeline-driver-queue/lib/validation/preference.js, line 13 at r17 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…
if ((position === 'first' || position === 'last') && query) {

Done.


core/pipeline-driver-queue/lib/validation/preference.js, line 26 at r17 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

You can say

 if (numberOfFields === 0) {
      throw new InvalidDataError('Query must contain one of jobId ,tag ,pipelineName');
}

Done.


core/pipeline-driver-queue/api/rest-api/routes/preferred.js, line 7 at r17 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

this is not async

Done.


core/pipeline-driver-queue/api/rest-api/routes/preferred.js, line 11 at r17 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…

This is not a best practice, it is confusing...
The user can set the same jobs ids in addedJobs and removedJobs...
I think you need to add router.delete with a body.

Done.

@golanha golanha requested a review from nassiharel December 23, 2021 07:09
@golanha
Copy link
Member Author

golanha commented Dec 23, 2021

/deploy

@hkube-ci hkube-ci temporarily deployed to dev December 23, 2021 11:19 Inactive
nassiharel
nassiharel previously approved these changes Dec 23, 2021
Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 87 of 104 files reviewed, 3 unresolved discussions (waiting on @nassiharel and @yehiyam)

@golanha
Copy link
Member Author

golanha commented Dec 26, 2021

/deploy

@hkube-ci hkube-ci temporarily deployed to dev December 26, 2021 08:34 Inactive
@golanha
Copy link
Member Author

golanha commented Dec 26, 2021

/deploy

@hkube-ci hkube-ci temporarily deployed to dev December 26, 2021 09:42 Inactive
@golanha golanha merged commit 4f35822 into master Dec 27, 2021
@golanha golanha deleted the prioritized_jobs branch December 27, 2021 07:07
hkube-ci pushed a commit that referenced this pull request Dec 27, 2021
* job priority

* Optimizer (#1430)

* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR

* Optimizer (#1430)

* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR .... bump version [skip ci]

* Optimizer (#1430)

* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR .... bump version [skip ci]

* job priority

* deep scan

* exclude failing test

* tests

* update db package version

* package-lock

* expose rest, use 2 queues

* update preference api

* remove redundant file

* error handling

* load data and tests

* CR

* CR

* api update

* upgrade consts

* test accoring to api changes

* ingress to driver-queue

* swagger ui path to driver-queue/preferred

* CR

* deepscan

* fix api test

* CR

* add delete test

* CR

* event only when realy dequeueing

* driver-queue -> queue

* dequeued length

* no metrics updates for preferred queue

Co-authored-by: GitHub Action <[email protected]> .... bump version [skip ci]
hkube-ci pushed a commit that referenced this pull request Dec 27, 2021
* job priority

* Optimizer (#1430)

* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR

* Optimizer (#1430)

* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR .... bump version [skip ci]

* Optimizer (#1430)

* Add algorithm-optimizer module

* Add optimizer to api server

* no lint

* Add Test

* remove mem

* increase memory

* optimizer schema

* validations

* sampler and validation on store

* correct test

* handle optunaboard

* optimizer pod deploymnet fixes

* lock file

* update consts

* volume mounts in spec

* missing argument in volume mounts

* storage as datasource

* env variable set

* set env variable

* env set correction

* rest api for optunaboard and algorithm-operator handle optunaboard

* deepscan

* boardURL

* rename optimizer folder to hyperparams-tuner

* optimizer to hyperparams-tuner

* optimizer to hyperparams-tuner

* get optunaboard version from values

* lint

* change validation

* CR

* Consts typo

* hyperparams not mandatory incase of grid sample

* test failed

* CR .... bump version [skip ci]

* job priority

* deep scan

* exclude failing test

* tests

* update db package version

* package-lock

* expose rest, use 2 queues

* update preference api

* remove redundant file

* error handling

* load data and tests

* CR

* CR

* api update

* upgrade consts

* test accoring to api changes

* ingress to driver-queue

* swagger ui path to driver-queue/preferred

* CR

* deepscan

* fix api test

* CR

* add delete test

* CR

* event only when realy dequeueing

* driver-queue -> queue

* dequeued length

* no metrics updates for preferred queue

Co-authored-by: GitHub Action <[email protected]> .... bump version [skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants