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 V1 endpoints for factory, machine, machine_type #24

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

mks-sight
Copy link

@mks-sight mks-sight commented Dec 6, 2022

Hi, Please review the PR, Thanks

https://sightmachine.atlassian.net/browse/MARS-7090

smsdk/client.py Outdated Show resolved Hide resolved
smsdk/client.py Outdated Show resolved Hide resolved
smsdk/client.py Outdated Show resolved Hide resolved
smsdk/client_v0.py Outdated Show resolved Hide resolved
@mks-sight mks-sight changed the title comment get_cycles, get_parts, get_downtimes methods for ClientV0 base method to use v1 endpints defined Add V1 endpoints for factory, machine, machine_type Jan 13, 2023
Copy link

@mklein0 mklein0 left a comment

Choose a reason for hiding this comment

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

Could unit tests against https://demo-continuous.sightmachine.io/ be added? I think we can have basic username/password added to that tenant. We wouild just need to add the password for our list of secrets in repo, so if it needs to be changed we are aware of it.

smsdk/client.py Outdated Show resolved Hide resolved
smsdk/client.py Outdated Show resolved Hide resolved
smsdk/client.py Outdated Show resolved Hide resolved
smsdk/client.py Show resolved Hide resolved
smsdk/client.py Outdated Show resolved Hide resolved
smsdk/utils.py Outdated Show resolved Hide resolved
@mklein0
Copy link

mklein0 commented Feb 8, 2023

A test using mocks against the requests library can be done with: https://requests-mock.readthedocs.io/en/latest/mocker.html

…er_by with select and order_by, priorities with single char
@mks-sight
Copy link
Author

mks-sight commented Feb 13, 2023

A test using mocks against the requests library can be done with: https://requests-mock.readthedocs.io/en/latest/mocker.html

Are we looking for new way of mocking request since monkeypatch of mock is already there or we are looking for an INTEGRATION test to test sdk code end to end

for eg.

`def test_get_machines(monkeypatch):
# Setup
def mockapi(self, session, endpoint):
if endpoint == "/api/machine":
return pd.DataFrame(JSON_MACHINE)
return pd.DataFrame()

monkeypatch.setattr(Machine, "get_machines", mockapi)

dt = Machine(Session(), "demo")

# Run
df = dt.get_machines(Session(), "/api/machine")

# Verify
assert df.shape == (27, 7)

cols = [
    "factory_location",
    "factory_partner",
    "id",
    "metadata",
    "source",
    "source_clean",
    "source_type",
]

assert cols == df.columns.sort_values().tolist()

`

in this code we are mocking, do we need to add more tests like this or do we need to use the sdk to test against demo-continuos from an end-user perspective?
Thanks

smsdk/client_v0.py Outdated Show resolved Hide resolved
smsdk/client_v0.py Outdated Show resolved Hide resolved
smsdk/config/api_endpoints.json Outdated Show resolved Hide resolved
smsdk/ma_session.py Outdated Show resolved Hide resolved
smsdk/ma_session.py Outdated Show resolved Hide resolved
smsdk/ma_session.py Outdated Show resolved Hide resolved
smsdk/ma_session.py Show resolved Hide resolved
smsdk/ma_session.py Outdated Show resolved Hide resolved
smsdk/utils.py Outdated Show resolved Hide resolved
smsdk/utils.py Outdated Show resolved Hide resolved
… the way exceptions were logged, add data type for check_kw, removed v0 endpoints
@mks-sight
Copy link
Author

A test using mocks against the requests library can be done with: https://requests-mock.readthedocs.io/en/latest/mocker.html

we are creating a separate ticket for this

@mks-sight mks-sight closed this Mar 27, 2023
@mks-sight mks-sight reopened this Mar 27, 2023
@mks-sight
Copy link
Author

A test using mocks against the requests library can be done with: https://requests-mock.readthedocs.io/en/latest/mocker.html

we are creating a separate ticket for this

@mks-sight
Copy link
Author

Could unit tests against https://demo-continuous.sightmachine.io/ be added? I think we can have basic username/password added to that tenant. We wouild just need to add the password for our list of secrets in repo, so if it needs to be changed we are aware of it.

we are creating a separate ticket for this

smsdk/Auth/auth.py Outdated Show resolved Hide resolved
smsdk/utils.py Outdated Show resolved Hide resolved
smsdk/client.py Outdated
Comment on lines 169 to 171
if util_name in ['get_factories', 'get_machines', 'get_machine_types']:
# data = dict_to_df(getattr(cls, util_name)(*args, **sub_kwargs[0]), normalize)
return getattr(cls, util_name)(normalize, *args, **sub_kwargs[0])
Copy link

Choose a reason for hiding this comment

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

FYI, It it the way the current SDK is designed, but I dislike the need to need to inspect the method names to determine function signatures rather than leveraging the language and OO methods.

smsdk/client.py Outdated
@@ -157,13 +160,17 @@ def get_data_v1(self, ename, util_name, normalize=True, *args, **kwargs):
# dict params strictly follow {'key':'value'} format

# sub_kwargs = kwargs
if util_name in ['get_cycles', 'get_downtime', 'get_parts']:
if util_name in ['get_cycles', 'get_downtime', 'get_parts', 'get_factories', 'get_machines', 'get_machine_types']:
Copy link

Choose a reason for hiding this comment

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

Unit Tests are a separate PR?

Copy link

Choose a reason for hiding this comment

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

A useful combination of tools to see how well the code created has test coverage is:

  • coverage
  • diff-cover

ex.

coverage run -m pytest -vvv -s  tests
coverage xml
diff-cover --compare-branch origin/master --html-report diff-report.html coverage.xml
xdg-open diff-report.html

The report given goes off the code vs the diff, but still useful.

smsdk/client.py Outdated Show resolved Hide resolved
@harshmishraintg
Copy link

I don't have any more comments on this. Approved

# raise ValueError("Error - {}".format(records))
return records

def modify_input_params(self, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something but it seems like all of these modify_input_params are the same. Is there a reason this isn't a util you import?

@@ -278,3 +277,87 @@ def get_starttime_endtime_keys(self, **kwargs):
continue

return starttime_key, endtime_key

def _get_records_mongo_v1(
Copy link

Choose a reason for hiding this comment

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

What does mongo in the function name mean?

Copy link

Choose a reason for hiding this comment

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

What is this module kept around for? What v0 interfaces remain?

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.

5 participants