Skip to content
This repository has been archived by the owner on Sep 22, 2023. It is now read-only.

Added --format and --plain options to the 'backend.ai ps' command #80

Merged
merged 8 commits into from
Jan 8, 2020

Conversation

miraliahmadli
Copy link
Contributor

@miraliahmadli miraliahmadli commented Jan 6, 2020

lablup/backend.ai#87
@achimnol

  • Added --format/-f options for "backend.ai ps"/"backend.ai admin session" commands

  • Added --plain option to see process status info in plain tabular format

  • I will update the documentation file as soon as possible

@codecov
Copy link

codecov bot commented Jan 6, 2020

Codecov Report

Merging #80 into master will increase coverage by 0.65%.
The diff coverage is 47.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   43.01%   43.67%   +0.65%     
==========================================
  Files          50       50              
  Lines        5145     5569     +424     
==========================================
+ Hits         2213     2432     +219     
- Misses       2932     3137     +205
Impacted Files Coverage Δ
src/ai/backend/client/cli/ps.py 93.75% <100%> (+0.89%) ⬆️
src/ai/backend/client/cli/admin/sessions.py 16.43% <35.71%> (+0.88%) ⬆️
src/ai/backend/client/cli/__init__.py 84.88% <0%> (+0.17%) ⬆️
src/ai/backend/client/session.py 94.37% <0%> (+1.43%) ⬆️
src/ai/backend/client/kernel.py 45.8% <0%> (+2.79%) ⬆️
src/ai/backend/client/cli/run.py 25.3% <0%> (+3.99%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2669a0...6fccf18. Read the comment docs.

Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

A few minor fixes please!

@@ -28,7 +28,7 @@ other users by adding ``--access-key`` option here.

backend.ai admin sessions

Both commands offers options to set the status filter as follows.
Both commands offer options to set the status filter as follows.
Copy link
Member

Choose a reason for hiding this comment

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

👍

``Max Used Memory (MiB)``, and ``CPU Using (%)``.

* - ``-f``, ``--format``
- ``Session ID``, ``Owner``, and specified fields by user.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to remove these two default-included fields for full customizability.
Such "templated" options are already there: --detail, --id-only.

Copy link
Member

Choose a reason for hiding this comment

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

Just show an error sayng "at least one field is required for custom format" if there is no fields given at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed that part. If user does not pass any argument, it shows error message saying that Error: --format option requires an argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though I print failure message "At least one field is required for custom format", it still prints the error message above. I think it is related to the click package.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, then it's okay to assume that there will be at least one item in the format argument.

.. note::
Fields for ``-f/--format`` option can be displayed by specifying comma-separated parameters.

Available parameters for this option are: ``id``, ``status``, ``status_info``, ``created_at``, ``last_updated``, ``result``, ``image``, ``type``, ``task_id``, ``tag``, ``occupied_slots``, ``used_memory``, ``max_used_memory``, ``cpu_using``.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -32,7 +51,9 @@
@click.option('-a', '--all', is_flag=True,
help='Display all sessions matching the condition using pagination.')
@click.option('--detail', is_flag=True, help='Show more details using more columns.')
def sessions(status, access_key, id_only, show_tid, dead, running, all, detail):
@click.option('-f', '--format', default=None, help='Display only specified fields.')
@click.option('--plain', is_flag=True, help='Display process status in plain format.')
Copy link
Member

Choose a reason for hiding this comment

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

"process status" looks somewhat ambiguous.
Let's say "Display the session list without decorative line drawings and the header."

Copy link
Member

Choose a reason for hiding this comment

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

Also don't forget to update the same option in ps.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

@miraliahmadli
Copy link
Contributor Author

Thank you for your feedback, @achimnol !!!

@achimnol achimnol merged commit 90737da into master Jan 8, 2020
@achimnol achimnol deleted the customized_cli branch January 8, 2020 03:20
@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants