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

Expose analyze endpoint #376

Merged

FedericoNegri merged 19 commits into main from fnegri/analyze

Apr 23, 2024

Contributor

FedericoNegri commented Apr 10, 2024

No description provided.

FedericoNegri added 15 commits

March 6, 2024 10:34


          update rms models

eb644f3


          draft: expose analyze endpoint

2ebcea0


          add custom reqs and queue

5b7dc13


          Merge branch 'main' into fnegri/analyze

b70e20b


          Merge branch 'main' into fnegri/analyze

8cbd1e2


          update models

2630c3c


          error handling

d68a0fd


          Merge branch 'main' into fnegri/analyze

960fe40


          update models


          Merge branch 'main' into fnegri/analyze

d6c585b


          update to latest rms

bcc694e


          clean up

bd91363


          Merge branch 'main' into fnegri/analyze

c5fef67


          update models and add test

ffc518d


          Merge branch 'main' into fnegri/analyze

b2349e6

FedericoNegri added the enhancement label

codecov bot commented Apr 10, 2024 •

edited

Loading

Codecov Report

Attention: Patch coverage is 97.67442% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 93.63%. Comparing base (0dcbc15) to head (3f32af3).

Files	Patch %	Lines
src/ansys/hps/client/rms/api/base.py	71.42%	2 Missing ⚠️
src/ansys/hps/client/jms/api/project_api.py	91.66%	1 Missing ⚠️
src/ansys/hps/client/rms/api/rms_api.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
+ Coverage   93.46%   93.63%   +0.17%     
==========================================
  Files          62       62              
  Lines        2279     2372      +93     
==========================================
+ Hits         2130     2221      +91     
- Misses        149      151       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/jms/api/project_api.py Outdated

Comment on lines 293 to 294

		# we get the task definition as a native dict to more easily translate
		# the sub-objects into RMS models

Member

PipKat Apr 10, 2024 •

edited

Loading

Suggested change

      
                    # we get the task definition as a native dict to more easily translate
          
                    # the sub-objects into RMS models
          
                    # Task definition is retrieved as a native dictionary to more easily translate
          
                    # the subobjects into RMS models

Avoid "we"--even in comments.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/api/rms_api.py Outdated

+                  ) -> AnalyzeResponse:
+                      """Compare resource requirements against available compute resources."""
+                      if requirements is None:
+                          raise ClientError(f"requirements can't be None.")

Member

PipKat Apr 10, 2024

Suggested change

      
                        raise ClientError(f"requirements can't be None.")
          
                        raise ClientError(f"Requirements can't be 'None'.")

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

+                  )
+                  number_accessible_assignable_and_matching: int = Field(
+                      ...,
+                      description='Number of workers matching hardware, platform, application, custom requirements, assignment and permissions',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Number of workers matching hardware, platform, application, custom requirements, assignment and permissions',
          
                    description='Number of workers matching hardware, platform, application, custom requirements, assignment, and permissions',

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

+              class AnalyzeHpcResources(BaseModel):
+                  num_cores_per_node: Optional[int] = Field(None, title='Num Cores Per Node')
+                  num_gpus_per_node: Optional[int] = Field(None, title='Num Gpus Per Node')

Member

PipKat Apr 10, 2024 •

edited

Loading

Suggested change

      
                num_gpus_per_node: Optional[int] = Field(None, title='Num Gpus Per Node')
          
                num_gpus_per_node: Optional[int] = Field(None, title='Num Gpus Per Node')

Do the descriptions that appear in "Field" show in the documentation? If so, they should be written out in plain English without using abbreviations. They should also conclude with periods. Later "Field" descriptions are formatted in the way that I expect.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

    
                  )

                  host_id: Optional[str] = Field(

                      None, description="Static, hardware and configuration-based UUID.", title="Host Id"

                      None, description='Static, hardware and configuration-based UUID.', title='Host Id'

Member

PipKat Apr 10, 2024

Suggested change

      
                    None, description='Static, hardware and configuration-based UUID.', title='Host Id'
          
                    None, description='Static hardware and configuration-based UUID.', title='Host Id'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

    
                  )

                  exclusive: Optional[bool] = Field(

                      None, description="To not share nodes with other running jobs.", title="Exclusive"

                      None, description='To not share nodes with other running jobs.', title='Exclusive'

Member

PipKat Apr 10, 2024

Suggested change

      
                    None, description='To not share nodes with other running jobs.', title='Exclusive'
          
                    None, description='Whether to not share nodes with other running jobs.', title='Exclusive'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

    
                  debug: Optional[bool] = Field(

                      False, description="Enable additional debugging of the backend", title="Debug"

                      False, description='Enable additional debugging of the backend', title='Debug'

Member

PipKat Apr 10, 2024

Suggested change

      
                    False, description='Enable additional debugging of the backend', title='Debug'
          
                    False, description='Whether to enable additional debugging of the backend', title='Debug'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

-                      description="Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment`` and ``statefulset``.",
-                      title="Target Resource Kind",
+                      'job',
+                      description='Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment`` and ``statefulset``.',

Member

PipKat Apr 10, 2024

Still using "REP" here?

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

-                      description="Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment`` and ``statefulset``.",
-                      title="Target Resource Kind",
+                      'job',
+                      description='Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment`` and ``statefulset``.',

Member

PipKat Apr 10, 2024 •

edited

Loading

Suggested change

      
                    description='Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment`` and ``statefulset``.',
          
                    description='Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment``, and ``statefulset``.',

We recommend alphabetizing the options, unless there is a reason not to. There are other occurrences, but I won't repeat this suggestion.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

-                      description="Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment`` and ``statefulset``.",
-                      title="Target Resource Kind",
+                      'job',
+                      description='Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment`` and ``statefulset``.',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment`` and ``statefulset``.',
          
                    description='Kubernetes resource kind that the REP scaler should target. Options are ``job``, ``deployment``, and ``statefulset``.',

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

    
                  debug: Optional[bool] = Field(

                      False, description="Enable additional debugging of the backend", title="Debug"

                      False, description='Enable additional debugging of the backend', title='Debug'

Member

PipKat Apr 10, 2024

Suggested change

      
                    False, description='Enable additional debugging of the backend', title='Debug'
          
                    False, description='Whether to enable additional debugging of the backend', title='Debug'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

    
                  )

              class LocalBackend(BaseModel):

                  plugin_name: Literal["local"] = Field(..., title="Plugin Name")

                  plugin_name: Literal['local'] = Field(..., title='Plugin Name')

Member

PipKat Apr 10, 2024

Suggested change

      
                plugin_name: Literal['local'] = Field(..., title='Plugin Name')
          
                plugin_name: Literal['local'] = Field(..., title='Plugin name.')

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

                   )
               class Machine(BaseModel):
-                  name: str = Field(..., description="Name of the machine", title="Name")
-                  num_cores: int = Field(..., description="Number of cores available", title="Num Cores")
+                  name: str = Field(..., description='Name of the machine', title='Name')

Member

PipKat Apr 10, 2024

These descriptions don't conclude with a period.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

+                      title='Scheduler Type',
+                  )
+                  enable_api: Optional[bool] = Field(
+                      False, description='Enable to use the scheduler Rest API feature', title='Enable Api'

Member

PipKat Apr 10, 2024

Suggested change

      
                    False, description='Enable to use the scheduler Rest API feature', title='Enable Api'
          
                    False, description='Whether to use the scheduler REST API feature.', title='Enable Api'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

                   )
+                  base_url: Optional[str] = Field(
+                      'http://localhost:5050', description='The Rest API URL', title='Base Url'

Member

PipKat Apr 10, 2024

Suggested change

      
                    'http://localhost:5050', description='The Rest API URL', title='Base Url'
          
                    'http://localhost:5050', description='REST API URL.', title='Base Url'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

+                  base_url: Optional[str] = Field(
+                      'http://localhost:5050', description='The Rest API URL', title='Base Url'
+                  )
+                  api_ver: Optional[str] = Field('v0.0.39', description='The Rest API version', title='Api Ver')

Member

PipKat Apr 10, 2024

Suggested change

      
                api_ver: Optional[str] = Field('v0.0.39', description='The Rest API version', title='Api Ver')
          
                api_ver: Optional[str] = Field('v0.0.39', description='REST API version.', title='Api Ver')

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

                   )
                   exclusive_default: Optional[bool] = Field(
                       False,
-                      description="Request the scheduler to hold the nodes exclusively for one request.",
-                      title="Exclusive Default",
+                      description='Request the scheduler to hold the nodes exclusively for one request.',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Request the scheduler to hold the nodes exclusively for one request.',
          
                    description='Whether the scheduler is to hold the nodes exclusively for one request.',

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

                   )
                   distributed_default: Optional[bool] = Field(
                       True,
-                      description="Allow the scheduler to provide multiple machines to fulfill the request.",
-                      title="Distributed Default",
+                      description='Allow the scheduler to provide multiple machines to fulfill the request.',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Allow the scheduler to provide multiple machines to fulfill the request.',
          
                    description='Whether the scheduler is to provide multiple machines to fulfill the request.',

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

-                      description="Use the templated versions of the scripts and write them to the working directory.",
-                      title="Use Templates",
+                  use_local_scratch: Optional[bool] = Field(
+                      False, description='To use local storage as working dir for jobs', title='Use Local Scratch'

Member

PipKat Apr 10, 2024

Suggested change

      
                    False, description='To use local storage as working dir for jobs', title='Use Local Scratch'
          
                    False, description='Whether to use local storage as the working directory for jobs.', title='Use Local Scratch'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

                   )
+                  local_scratch_dir: Optional[str] = Field(
+                      None,
+                      description='Path to the local scratch directory to be used as jobs working dir',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Path to the local scratch directory to be used as jobs working dir',
          
                    description='Path to the local scratch directory to use as the job's working directory.',

I realized that you may have intended plural possessive (to use as the jobs' working directory). Please change if so.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

-                      discriminator="plugin_name",
-                      title="Scaling Strategy",
+                      {'debug': False, 'plugin_name': 'local'},
+                      description='Backend to use in this compute resource set.',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Backend to use in this compute resource set.',
          
                    description='Backend to use in the compute resource set.',

We prefer using "the" over "this" where possible.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

+                  scaling_strategy: Optional[Union[MaxAvailableResourceScaling, KubernetesResourceScaling]] = (
+                      Field(
+                          {'match_all_requirements': False, 'plugin_name': 'max_available_resource_scaling'},
+                          description='Scaling strategy to use in this compute resource set.',

Member

PipKat Apr 10, 2024

Suggested change

      
                        description='Scaling strategy to use in this compute resource set.',
          
                        description='Scaling strategy to use in the compute resource set.',

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

                   )
                   evaluator_auto_shutdown_time: Optional[int] = Field(
 ,
-                      description="Time after which to shutdown the evaluator if not running any jobs.",
-                      title="Evaluator Auto Shutdown Time",
+                      description='Time after which to shutdown the evaluator if not running any jobs.',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Time after which to shutdown the evaluator if not running any jobs.',
          
                    description='Time after which to shut down the evaluator if not running any jobs.',

Shut down is two words as a verb and one word as a noun.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

    
                  )

              class ComputeResourceSetsRequest(BaseModel):

                  compute_resource_sets: List[ComputeResourceSet] = Field(

                      ..., description="Compute resource set details", title="Compute Resource Sets"

                      ..., description='Compute resource set details', title='Compute Resource Sets'

Member

PipKat Apr 10, 2024

Suggested change

      
                    ..., description='Compute resource set details', title='Compute Resource Sets'
          
                    ..., description='Compute resource set details.', title='Compute Resource Sets'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

    
                  )

              class EvaluatorConfiguration(BaseModel):

                  id: Optional[str] = Field(None, description="Unique database ID (read-only).", title="Id")

                  id: Optional[str] = Field(None, description='Unique database ID (read-only).', title='Id')

Member

PipKat Apr 10, 2024

Can the title be ID rather than Id? We tend to always use ID.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

    
                  )

                  last_modified: Optional[datetime] = Field(

                      None, description="Last modified time", title="Last Modified"

                      None, description='Last modified time', title='Last Modified'

Member

PipKat Apr 10, 2024

Suggested change

      
                    None, description='Last modified time', title='Last Modified'
          
                    None, description='Last modified time.', title='Last Modified'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

                   )
                   project_server_select: Optional[bool] = Field(
                       True,
-                      description="Get project assignments from the server instead of using the locally set values",
-                      title="Project Server Select",
+                      description='Get project assignments from the server instead of using the locally set values',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Get project assignments from the server instead of using the locally set values',
          
                    description='Get project assignments from the server instead of using the locally set values.',

Usually, descriptions starting with verbs are reserved for classes and methods. I'm not sure how to rewrite this so. Would this work: Projects assignments to get from the server rather than using locally set values."

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

                   )
                   project_list: Optional[List[str]] = Field(
                       [],
-                      description="IDs of projects that the evaluator should work on, in order",
-                      title="Project List",
+                      description='IDs of projects that the evaluator should work on, in order',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='IDs of projects that the evaluator should work on, in order',
          
                    description='IDs of projects that the evaluator should work on, in order.',

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

-                      description="Specifies how the evaluator selects projects to work on.             One of: disabled, all_active, list",
-                      title="Project Assignment Mode",
+                      'all_active',
+                      description='Specifies how the evaluator selects projects to work on.             One of: disabled, all_active, list',

Member

PipKat Apr 10, 2024

Suggested change

      
                    description='Specifies how the evaluator selects projects to work on.             One of: disabled, all_active, list',
          
                    description='How the evaluator selects projects to work on.             Options are ``disabled`` and ``all_active, list'`,

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

    
                  )

                  last_modified: Optional[datetime] = Field(

                      None, description="Last modified time", title="Last Modified"

                      None, description='Last modified time', title='Last Modified'

Member

PipKat Apr 10, 2024

Suggested change

      
                    None, description='Last modified time', title='Last Modified'
          
                    None, description='Last modified time.', title='Last Modified'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py Outdated

                   name: Optional[str] = Field(
                       None,
-                      description="Update the name of the evaluator (updating the registration).",
-                      title="Name",
+                      description='Update the name of the evaluator (updating the registration).',

Member

PipKat Apr 10, 2024

Again--the description is normally a noun. Rather than starting with a verb, can we rewrite as: "Name to update the evaluator to. Chaning the name updates the registration."

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

                   )
                   project_assignment_mode: Optional[str] = Field(
                       None,
-                      description="How the evaluator selects projects to work on. Options are: disabled, all_active, list.",
-                      title="Project Assignment Mode",
+                      description='How the evaluator selects projects to work on. Options are: disabled, all_active, list.',

Member

PipKat Apr 10, 2024

Same suggestions as earlier.

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

    
                  )

              class EvaluatorConfigurationUpdatesResponse(BaseModel):

                  configuration_updates: List[EvaluatorConfigurationUpdate] = Field(

                      ..., description="Configuration update details", title="Configuration Updates"

                      ..., description='Configuration update details', title='Configuration Updates'

Member

PipKat Apr 10, 2024

Suggested change

      
                    ..., description='Configuration update details', title='Configuration Updates'
          
                    ..., description='Configuration update details.', title='Configuration Updates'

PipKat reviewed

View reviewed changes

src/ansys/hps/client/rms/models.py

    
                  )

              class EvaluatorConfigurationsResponse(BaseModel):

                  configurations: List[EvaluatorConfiguration] = Field(

                      ..., description="Evaluator configurations", title="Configurations"

                      ..., description='Evaluator configurations', title='Configurations'

Member

PipKat Apr 10, 2024

Suggested change

      
                    ..., description='Evaluator configurations', title='Configurations'
          
                    ..., description='Evaluator configurations.', title='Configurations'

PipKat reviewed

View reviewed changes

tests/jms/test_task_definition.py Outdated

Comment on lines 258 to 259

		# we can't assume compute resources to be available,
		# so we just hit the endpoint

Member

PipKat Apr 10, 2024

Suggested change

      
                # we can't assume compute resources to be available,
          
                # so we just hit the endpoint
          
                # Because compute resources can't be assumed to be available,
          
                # hit the endpoint

FedericoNegri added 4 commits

April 23, 2024 14:02


          Merge branch 'main' into fnegri/analyze

52963e2


          incorporate kathy's corrections

aec3e6c


          Merge branch 'main' into fnegri/analyze

a22756d


          add custom header for rms models

3f32af3

github-actions bot added the documentation label

FedericoNegri marked this pull request as ready for review

April 23, 2024 12:39

FedericoNegri merged commit 5bb88c8 into main

21 checks passed

FedericoNegri deleted the fnegri/analyze branch

April 23, 2024 14:02

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

Labels

documentation enhancement