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

Add main XY (TSP) endpoints support to client with tests #28

Merged
merged 4 commits into from
Sep 15, 2021
Merged

Add main XY (TSP) endpoints support to client with tests #28

merged 4 commits into from
Sep 15, 2021

Conversation

marco-miller
Copy link
Contributor

@marco-miller marco-miller commented Aug 31, 2021

Signed-off-by: Marco Miller [email protected]

test_tsp.py Outdated Show resolved Hide resolved
Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

UX issue, we need a way to get the times of the trace first.

List-experiment works, but it is not obvious to a new user

Also the fetch-xy unit test fails on my machine. Is this known, should it work?

FAILED test_tsp.py::TestTspClient::test_fetch_xy - AttributeError: 'NoneType' object has no attribute 'status'

Asside from that I approve this, I'll put a trusting +2 assuming the questions are answered. Even if these comments are accidentally not read, no harm, the issues are minor compared to the improvements this patch brings in.

tsp-cli-client Show resolved Hide resolved
@marco-miller
Copy link
Contributor Author

UX issue, we need a way to get the times of the trace first.
List-experiment works, but it is not obvious to a new user.

These follow the current TSP spec.
Would you be able to back-log a follow-up for us to cover?

Also the fetch-xy unit test fails on my machine. Is this known, should it work?

FAILED test_tsp.py::TestTspClient::test_fetch_xy - AttributeError: 'NoneType' object has no attribute 'status'

I added an assert in the latest patch, but this works locally for me.
Are you using the current latest master incubator server?

Aside from that I approve this, I'll put a trusting +2 assuming the questions are answered. Even if these comments are accidentally not read, no harm, the issues are minor compared to the improvements this patch brings in.

Thanks! :)

Copy link
Contributor

@MatthewKhouzam MatthewKhouzam left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Collaborator

@bhufmann bhufmann 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 the contribution. It is important for the python client, but also important to find problems with the server and TSP definition.

I have some initial comments... some items can be backlogged for after this PR

tsp-cli-client Outdated Show resolved Hide resolved
tsp/tsp_client.py Outdated Show resolved Hide resolved
tsp-cli-client Show resolved Hide resolved
tsp-cli-client Show resolved Hide resolved
tsp-cli-client Outdated Show resolved Hide resolved
@marco-miller
Copy link
Contributor Author

marco-miller commented Sep 13, 2021

My latest force-push (diff linked from right above) only did this:

  1. move the base trivial commit to another (more related) PR of mine;
  2. remove the tip commit as it depended on a now abandoned server patch (backlog material).

Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

One more change request. And then when the trace server fixes are merged, we can merge this PR.

tsp/tsp_client.py Outdated Show resolved Hide resolved
Do not rename the existing --get-tree cli script option, to preserve
backward compatibility with potential usage of it. Only use an 'xy'
infix in the name of the hereby added option, discriminating it from
that existing one for timegraph.

Assume EntryModel's default XY_TREE model_type while initializing the
GenericResponse for this case.

Update README's own copy of ./tsp-cli-client -h, accordingly.

Signed-off-by: Marco Miller <[email protected]>
Add a --get-xy cli script option along with its --items and --times
siblings. Update README's copy of script's -h, accordingly. Make that
script able to print the resulting XYModel contents as applicable.

Add a test, along with REQUESTED_TIME_XY value constants matching its
kernel test trace used; this is the trace that is already used across
all of those tests. Make that new test wait for its analysis to be
done running (so, complete or else), prior to try accessing its XY data.
The latter is otherwise not accessible yet. Limit this test scope depth
to what the sibling tests currently cover. (Deeper coverage backlogged.)

Move related constants as TspClient members so they are reused. Unlike
existing TspClient methods, the hereby added one requires parameters, as
it cannot infer default values for those in that specific case. Existing
default requested_times values in TspClient may require revisiting.

Signed-off-by: Marco Miller <[email protected]>
While outputting a corresponding message should this happen. Have that
code exit right away in the latter case, instead of not handling this.

Use the past tense in that message, as the model may become available
after a short while, if retried.

Signed-off-by: Marco Miller <[email protected]>
Replace these previously ineffective defaults with none, if none passed.

Signed-off-by: Marco Miller <[email protected]>
Copy link
Collaborator

@bhufmann bhufmann left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much for the contribution.

@marco-miller marco-miller merged commit 8991160 into eclipse-cdt-cloud:master Sep 15, 2021
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.

3 participants