-
Notifications
You must be signed in to change notification settings - Fork 0
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
Task/WP-80 Implement dynamic execution system selection #921
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #921 +/- ##
==========================================
+ Coverage 64.75% 65.40% +0.64%
==========================================
Files 434 437 +3
Lines 12528 12653 +125
Branches 2626 2667 +41
==========================================
+ Hits 8113 8276 +163
+ Misses 4179 4140 -39
- Partials 236 237 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
+ get available systems + handle client side scenarios
836f65f
to
95558cf
Compare
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.
This is really cool. A few questions
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.
Thanks for reviewing, addressed review comments in this new commit.
+ get available systems + handle client side scenarios
32a87c9
to
ffaf67e
Compare
data = {'definition': app_def} | ||
|
||
# GET EXECUTION SYSTEM INFO TO PROCESS SPECIFIC SYSTEM DATA E.G. QUEUE INFORMATION | ||
data['exec_sys'] = tapis.systems.getSystem(systemId=app_def.jobAttributes.execSystemId) | ||
exec_systems = getattr(app_def.notes, 'dynamicExecSystems', TapisResult()).__dict__.keys() |
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.
Should dynamicExecSystems
be optional? or have an "ALL" param? Or do we prefer the explicit dynamic exec systems? We may have discussed before, apologies if so
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 we landed on keeping it optional.
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.
Rather, I mean if dynamicExecSystem
is true
, and dynamicExecSystems
is undefined, should that mean that we do no filtering on the systems listed in the dropdown?
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.
Oh, I see. We wanted to see if one attribute could provide everything.
So, if we choose dynamicExecSystems, then:
- dynamicExecSystems: ["All"] - would do no filtering, would pick all systems available to the user.
- "dynamicExecSystems": map of systems to system label will limit to these systems only.
I can make that change, it would be useful for some apps and not require provisioning of app if a new system is added.
How would we want to track labels in this case. An example is ls6 - exec systems are defined as ls6. There is no end user friendly label is system definition. The only other way is to maintain a global list of system names to label.
This would fit names of well known system names and their labels. Should use notes field in system definition for label?
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.
@rstijerina - The fix for this is already in, please let me know if this addressed this issue.
There will be only 1 property dynamicExecSystems
, it will be handled:
- if the property does not exist, then there the exec system is static and used as defined in app json.
- If
dynamicExecSystems
isAll
, then all the available to the user are provided in the UI. - If
dynamicExecSystems
is a list of exec systems, then the list is used if the user has access to the system.
…rtal into task/WP-80-execution-system
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.
Looks good, and testing steps work well! I did notice when flipping between queues that requested nodes' default value does change to be valid for that queue, but maybe unexpectedly - for example, picking Frontera's large
queue sets the requested nodes to the min value for this queue, 513, and switching to the normal
queue sets this value to 512, the max value for this queue. Is this intentional? My assumption would be to set the value to the min for a given queue.
Thanks @edmondsgarrett for reviewing. The node count dependency on queue logic did not change with this PR. You can see that in production here : https://cep.tacc.utexas.edu/workbench/applications/opensees-mp-v35. The logic is to do |
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.
Thanks for that add info on the clamping, LGTM!
* Working version of execution system changes + get available systems + handle client side scenarios * Add tests, fix issues found from unit tests * Fix job history and also lint * Use one attribute for exec systems instead of two * Fix formatting * Sort system list in UI * Address code review comments * Adjusted exec system label text and fixed jest tests * Working version of execution system changes + get available systems + handle client side scenarios * Add tests, fix issues found from unit tests * Fix job history and also lint * Use one attribute for exec systems instead of two * Fix formatting * Sort system list in UI * Address code review comments * Adjusted exec system label text and fixed jest tests * Fix bug related to job history execution system * Merge fix * Redo the fix on job history * Rework commit for exec and allocation * Prettier, lint and test fix * Fix merge issues * Make exec system dependent on allocation * Fix job history display of system name * Handle max memory on a system * Validation for execSystemId and fix express VM job submission --------- Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com>
* Fix and enable shared workspaces unit test (#927) * Fix and enable shared workspaces unit test * Remove submodule added in a previous PR * WP-190: Handle concurrency with Tapis OAuth Token Refresh initial commit * WP-190: Handle concurrency with Tapis OAuth Token Refresh initial commit * Update server/portal/apps/auth/models.py Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> * remove unnecessary comment/todo * Handle validation for FORK job type (#946) * Handle validation for FORK job type * Prettier fix * Remove unrelated fix * Adjust Yup validation and initialValues * WP-551: use only strings for app arguments (#948) * build(deps): bump sqlparse from 0.4.4 to 0.5.0 in /server (#947) Bumps [sqlparse](https://github.com/andialbrecht/sqlparse) from 0.4.4 to 0.5.0. - [Changelog](https://github.com/andialbrecht/sqlparse/blob/master/CHANGELOG) - [Commits](andialbrecht/sqlparse@0.4.4...0.5.0) --- updated-dependencies: - dependency-name: sqlparse dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chandra Y <cyemparala@tacc.utexas.edu> * build(deps): bump idna from 3.4 to 3.7 in /server (#945) Bumps [idna](https://github.com/kjd/idna) from 3.4 to 3.7. - [Release notes](https://github.com/kjd/idna/releases) - [Changelog](https://github.com/kjd/idna/blob/master/HISTORY.rst) - [Commits](kjd/idna@v3.4...v3.7) --- updated-dependencies: - dependency-name: idna dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chandra Y <cyemparala@tacc.utexas.edu> * build(deps): bump werkzeug from 3.0.1 to 3.0.3 in /server (#950) Bumps [werkzeug](https://github.com/pallets/werkzeug) from 3.0.1 to 3.0.3. - [Release notes](https://github.com/pallets/werkzeug/releases) - [Changelog](https://github.com/pallets/werkzeug/blob/main/CHANGES.rst) - [Commits](pallets/werkzeug@3.0.1...3.0.3) --- updated-dependencies: - dependency-name: werkzeug dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * --- (#951) updated-dependencies: - dependency-name: requests dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps-dev): bump braces from 3.0.2 to 3.0.3 in /client (#952) Bumps [braces](https://github.com/micromatch/braces) from 3.0.2 to 3.0.3. - [Changelog](https://github.com/micromatch/braces/blob/master/CHANGELOG.md) - [Commits](micromatch/braces@3.0.2...3.0.3) --- updated-dependencies: - dependency-name: braces dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chandra Y <cyemparala@tacc.utexas.edu> * build(deps): bump urllib3 from 1.26.18 to 1.26.19 in /server (#953) Bumps [urllib3](https://github.com/urllib3/urllib3) from 1.26.18 to 1.26.19. - [Release notes](https://github.com/urllib3/urllib3/releases) - [Changelog](https://github.com/urllib3/urllib3/blob/1.26.19/CHANGES.rst) - [Commits](urllib3/urllib3@1.26.18...1.26.19) --- updated-dependencies: - dependency-name: urllib3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chandra Y <cyemparala@tacc.utexas.edu> * build(deps-dev): bump ws from 7.5.7 to 7.5.10 in /client (#954) Bumps [ws](https://github.com/websockets/ws) from 7.5.7 to 7.5.10. - [Release notes](https://github.com/websockets/ws/releases) - [Commits](websockets/ws@7.5.7...7.5.10) --- updated-dependencies: - dependency-name: ws dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chandra Y <cyemparala@tacc.utexas.edu> * build(deps): bump certifi from 2023.7.22 to 2024.7.4 in /server (#956) Bumps [certifi](https://github.com/certifi/python-certifi) from 2023.7.22 to 2024.7.4. - [Commits](certifi/python-certifi@2023.07.22...2024.07.04) --- updated-dependencies: - dependency-name: certifi dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump django from 4.2.11 to 4.2.14 in /server (#957) Bumps [django](https://github.com/django/django) from 4.2.11 to 4.2.14. - [Commits](django/django@4.2.11...4.2.14) --- updated-dependencies: - dependency-name: django dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump setuptools from 68.2.2 to 70.0.0 in /server (#958) Bumps [setuptools](https://github.com/pypa/setuptools) from 68.2.2 to 70.0.0. - [Release notes](https://github.com/pypa/setuptools/releases) - [Changelog](https://github.com/pypa/setuptools/blob/main/NEWS.rst) - [Commits](pypa/setuptools@v68.2.2...v70.0.0) --- updated-dependencies: - dependency-name: setuptools dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chandra Y <cyemparala@tacc.utexas.edu> * task/wp-594: Setup internal docs with login access (#959) * Allow docs to be behind login * Lint fix * Customize url path * Fix unit test * conditionally add urls.py * Task/WP-585: TAS API: Country is no longer available. (#955) * TAS: Country is no longer available. * Adjust tests * Search index cron tab fixes. (#949) * build(deps): bump django from 4.2.14 to 4.2.15 in /server (#960) Bumps [django](https://github.com/django/django) from 4.2.14 to 4.2.15. - [Commits](django/django@4.2.14...4.2.15) --- updated-dependencies: - dependency-name: django dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chandra Y <cyemparala@tacc.utexas.edu> * build(deps): bump twisted from 23.10.0 to 24.7.0 in /server (#961) Bumps [twisted](https://github.com/twisted/twisted) from 23.10.0 to 24.7.0. - [Release notes](https://github.com/twisted/twisted/releases) - [Changelog](https://github.com/twisted/twisted/blob/trunk/NEWS.rst) - [Commits](twisted/twisted@twisted-23.10.0...twisted-24.7.0) --- updated-dependencies: - dependency-name: twisted dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Task/WP-80 Implement dynamic execution system selection (#921) * Working version of execution system changes + get available systems + handle client side scenarios * Add tests, fix issues found from unit tests * Fix job history and also lint * Use one attribute for exec systems instead of two * Fix formatting * Sort system list in UI * Address code review comments * Adjusted exec system label text and fixed jest tests * Working version of execution system changes + get available systems + handle client side scenarios * Add tests, fix issues found from unit tests * Fix job history and also lint * Use one attribute for exec systems instead of two * Fix formatting * Sort system list in UI * Address code review comments * Adjusted exec system label text and fixed jest tests * Fix bug related to job history execution system * Merge fix * Redo the fix on job history * Rework commit for exec and allocation * Prettier, lint and test fix * Fix merge issues * Make exec system dependent on allocation * Fix job history display of system name * Handle max memory on a system * Validation for execSystemId and fix express VM job submission --------- Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> * WP-190: Handle concurrency with Tapis OAuth Token Refresh initial commit * WP-190: Handle concurrency with Tapis OAuth Token Refresh initial commit * Setup Middleware for tapis token refresh --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> Co-authored-by: Garrett Edmonds <43251554+edmondsgarrett@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Overview
There are exec system agnostic apps, and the Portal Job submission form needs to allow the users to pick an execution system for such apps. This PR implements the feature to provide that.
Implementation decisions:
UI Visibility of execution systems:
The alternative is to show all execution systems available to user, which is not going to work well because user will blocked by allocation.
Node and Time limits when exec system is flipped:
When exec system selection is flipped in the form, should the node and time limits match what's in the app defaults or should it keep the user modifications to those defaults.
Bring the app defaults:
Steps:
After step 4, node count goes back to 1 and default queue for ls6.
Keep the current node count on changing exec system (the PR implemented this)
Steps:
After step 4, node count adjust to ls6 default queue and will not go back to 1.
Cannot use tapis job attribute dynamicExecSystem as an indicator of whether app supports dynamic exec system.
Currently dynamic selection of an execution system is not supported. For this reason the job related attribute dynamicExecSystem should be set to false (the default) and execSystemConstraints should not be set.
dynamicExecSystem
Related
Changes
Backend:
Dependent picklist
Add a notes option for each app, to allow a selection of dynamic execution systems. Existance of this attribute indicates that dynamic execution system feature is available. There are two ways to provide dynamic execution systems.
Example:
Specific list of execution systems. Only LoneStar6, Frontera are shown if the user has access to them.
execSystems
.Client:
Testing
App with no dynamicExecSystem in notes section:
App with dynamic execution system
Pre-req:
dynamicExecSystems: ["ls6", "frontera"]
Flip between frontera and ls6. Check the following:
Use an allocation with no available execution systems and see what happens. (I only saw 3DEM based allocation with no hosts).
Test automation was added/updated for the following scenarios:
all exec systems with allocation
some exec systems with no allocation
only one exec system with allocation
only one exec system with allocation
no exec system with allocation
2 exec systems with missing default queue
2 exec systems with different queue names
2 exec systems - queue selection and changing exec system
2 exec systems - modify queue selection and check the corresponding dependencies
2 exec systems - modify queue limits and check if they are set appropriately in the values
2 exec systems - modify queue on new exec system
Verification:
UI
Order of drop down is different. Allocation is first and then Systems.

Added drop down to show exec system with labels from system.

Error message when allocation has no execution system.
Notes