DOM-35464 convert passed in hardware tier name to hardware tier id #117

ddl-awroblicky · 2022-01-14T00:32:57Z

still working on adding tests

dknupp

Thanks for the quick response on this. I think we need to get away from the hardware tier name entirely, however, for reasons noted.

Also, note that there have been recent efforts to add unit tests for any new changes to the library. My gut is that some testing could be added here:
https://github.com/dominodatalab/python-domino/blob/master/tests/test_jobs.py

dknupp · 2022-01-14T00:37:09Z

domino/domino.py

@@ -818,6 +819,12 @@ def hardware_tiers_list(self):
        url = self._routes.hardware_tiers_list(self._project_id)
        return self._get(url)

+    def get_hardware_tier_id_from_name(self):


AFAIK, hardware tier names are not required to be unique, so this will be an issue. I think this is why overriding by name was probably a mistake in the first place.

It's unfortunate, but I think to address this issue, we really need to change the interface of job_start() to take hardware ID from the start. At least we know that no customer is using this as is (because it's totally broken, and no one has reported a bug before now).

An alternative to changing the interface would be to accept the name as exists right now, get the ID like you proposed, but throw an error in the event that more than one HWT with the same name is found.

I'm honestly not sure which is the better option.

perhaps similar to on_demand_spark_cluster_properties, I can document HWT name as deprecated and try the best effort approach and fail if multiple HWT with the same name are found. This way if any customer is using it but not noticing it doesn't work, their code will at least still run.

I can also add a new optional property for HWT id that is recommended to be used instead.

@dknupp Does this approach seem ok?

@ddl-awroblicky -- ah, I saw this was still marked as a draft, so I didn't know if there were more commits coming. I'll try to have a look later this afternoon or tomorrow.

dknupp · 2022-01-26T19:42:11Z

@ddl-awroblicky -- thanks for adding the unit tests!

I'll try to review this today.

dknupp · 2022-01-27T08:10:46Z

domino/domino.py

@@ -425,12 +432,13 @@ def validate_is_external_volume_mounts_supported():
                "masterHardwareTierId": master_hardware_tier_id
            }

+        resolved_hardware_tier_id = hardware_tier_id if hardware_tier_id is not None else self.get_hardware_tier_id_from_name(hardware_tier_name)


You might be able to shorten this line with...

Suggested change

resolved_hardware_tier_id = hardware_tier_id if hardware_tier_id is not None else self.get_hardware_tier_id_from_name(hardware_tier_name)

resolved_hardware_tier_id = hardware_tier_id or self.get_hardware_tier_id_from_name(hardware_tier_name)

dknupp · 2022-01-27T08:23:18Z

domino/routes.py

@@ -150,6 +150,12 @@ def app_get(self, app_id):
        return f'{self.host}/v4/modelProducts/{app_id}'

    # Hardware Tier URLs
+    def hardware_tier_create(self, hardware_tier_id, hardware_tier_name, node_pool):
+        return self.host + f'/admin/hwtiers/insert'


Can admin endpoints only be called by admin users? I don't honestly know.

yes that should be the case

if not it is a bug

dknupp · 2022-01-27T08:25:18Z

tests/test_hardware_tiers.py

+    """
+    Confirm that the python-domino client can create a new project.
+    """
+    new_project_name = f"project-{str(uuid.uuid4())}"


This test is creating a project, not a hardware tier. Maybe a copy/paste error?

dknupp · 2022-01-27T08:28:45Z

tests/test_hardware_tiers.py

+    default_domino_client.project_archive(new_project_name)
+
+@pytest.mark.skipif(not domino_is_reachable(), reason="No access to a live Domino deployment")
+def test_archiving_non_existent_hardware_tier_raises_appropriate_error(dummy_hostname, requests_mock):


Similarly, this test still seems to be archiving a project, not a hardware tier.

dknupp · 2022-01-27T08:34:55Z

tests/test_jobs.py

+    hardware_tiers = default_domino_client.hardware_tiers_list(created_project['id'])
+    non_default_hardware_tiers = (hardware_tier["isDefault"] == False for hardware_tier in hardware_tiers)
+
+    if len(non_default_hardware_tiers) > 0:


I'm surprised len() isn't throwing an exception here, something like...

>>> num_generator = (n for n in [1,2,3]) >>> len(num_generator) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: object of type 'generator' has no len()

dknupp · 2022-01-27T08:35:40Z

tests/test_jobs.py

+    created_project = next((p for p in project_list if p['name'] == new_project_name), None)
+
+    hardware_tiers = default_domino_client.hardware_tiers_list(created_project['id'])
+    non_default_hardware_tiers = (hardware_tier["isDefault"] == False for hardware_tier in hardware_tiers)


This test ran and passed? The way I read the current line, non_default_hardware_tiers is being assigned a python generator that will produce a series of Boolean values. It looks like you wanted a list comprehension instead, something like...

Suggested change

non_default_hardware_tiers = (hardware_tier["isDefault"] == False for hardware_tier in hardware_tiers)

non_default_hardware_tiers = [hwt for hwt in hardware_tiers if not hwt["isDefault"]]

dknupp

There is a test regression here. On master, if I run the tests which do not require a live host (i.e., pure unit test), they pass.

(venv) /path/to/dominodatalab/python-domino(master|…) % pytest -sv tests/test_jobs.py

[...]

tests/test_jobs.py::test_job_status_completes_with_default_params PASSED
tests/test_jobs.py::test_job_status_ignores_RequestException_and_times_out PASSED
tests/test_jobs.py::test_job_status_without_ignoring_exceptions PASSED

However, if I checkout your branch and run them, they fail.

(venv) /path/to/dominodatalab/python-domino(fix_hardware_tier_name_override↑1|…) % pytest -sv tests/test_jobs.py

[...]

tests/test_jobs.py::test_job_status_completes_with_default_params FAILED
tests/test_jobs.py::test_job_status_ignores_RequestException_and_times_out FAILED
tests/test_jobs.py::test_job_status_without_ignoring_exceptions FAILED

I'm trying to dig into why this is happening.

In the meantime, here are some other minor changes that need to be made.

tests/test_jobs.py

dknupp · 2022-03-01T02:08:05Z

tests/test_jobs.py

+        job_status = default_domino_client.job_start_blocking(command="main.py", hardware_tier_name=override_hardware_tier_name)
+        assert job_status['statuses']['isCompleted'] is True
+        job_red = default_domino_client.job_runtime_execution_details(job_status['id'])
+        assert job_red["hardwareTier"] == override_hardware_tier_name

 @pytest.mark.skipif(not domino_is_reachable(), reason="No access to a live Domino deployment")


PEP-8 requires 2 lines between module-level functions.

Suggested change

@pytest.mark.skipif(not domino_is_reachable(), reason="No access to a live Domino deployment")

@pytest.mark.skipif(not domino_is_reachable(), reason="No access to a live Domino deployment")

@ddl-awroblicky can you update the CHANGELOG.md?

dknupp · 2022-03-01T02:09:34Z

tests/test_jobs.py

+        job_red = default_domino_client.job_runtime_execution_details(job_status['id'])
+        assert job_red["hardwareTierId"] == override_hardware_tier_id
+
+# deprecated but ensuring it still works for now


PEP-8 requires 2 lines between module-level functions.

Suggested change

# deprecated but ensuring it still works for now

# deprecated but ensuring it still works for now

dknupp · 2022-03-01T02:22:36Z

tests/test_jobs.py

+    if len(list(non_default_hardware_tiers)) > 0:
+        override_hardware_tier_name = non_default_hardware_tiers[0]["hardwareTier"]["name"]
+        job_status = default_domino_client.job_start_blocking(command="main.py", hardware_tier_name=override_hardware_tier_name)
+        assert job_status['statuses']['isCompleted'] is True
+        job_red = default_domino_client.job_runtime_execution_details(job_status['id'])
+        assert job_red["hardwareTier"] == override_hardware_tier_name


The way this is written, it leaves open the possibility of a falsely passing test. If len(non_default_hardware_tiers) == 0, then the actual test never runs -- but it will still appear to be "passing." Reverse the logic and xfail() if the test can't run.

Suggested change

if len(list(non_default_hardware_tiers)) > 0:

override_hardware_tier_name = non_default_hardware_tiers[0]["hardwareTier"]["name"]

job_status = default_domino_client.job_start_blocking(command="main.py", hardware_tier_name=override_hardware_tier_name)

assert job_status['statuses']['isCompleted'] is True

job_red = default_domino_client.job_runtime_execution_details(job_status['id'])

assert job_red["hardwareTier"] == override_hardware_tier_name

if len(list(non_default_hardware_tiers)) > 0:

pytest.xfail("No non-default hardware tiers found: cannot run test")

override_hardware_tier_name = non_default_hardware_tiers[0]["hardwareTier"]["name"]

job_status = default_domino_client.job_start_blocking(command="main.py",

hardware_tier_name=override_hardware_tier_name)

assert job_status['statuses']['isCompleted'] is True

job_red = default_domino_client.job_runtime_execution_details(job_status['id'])

assert job_red["hardwareTier"] == override_hardware_tier_name

dknupp · 2022-03-01T02:44:52Z

tests/test_jobs.py

+    """
+    hardware_tiers = default_domino_client.hardware_tiers_list()
+    non_default_hardware_tiers = [hwt for hwt in hardware_tiers if not hwt["hardwareTier"]["isDefault"]]
+    if len(list(non_default_hardware_tiers)) > 0:


This should already be a list, so the call to list() here is redundant.

dknupp · 2022-03-01T02:45:52Z

tests/test_jobs.py

+    """
+    hardware_tiers = default_domino_client.hardware_tiers_list()
+    non_default_hardware_tiers = [hwt for hwt in hardware_tiers if not hwt["hardwareTier"]["isDefault"]]
+    if len(list(non_default_hardware_tiers)) > 0:


This should already be a list, so no need to call list().

dknupp

Thanks @ddl-awroblicky!

dknupp · 2022-03-14T20:27:44Z

@ddl-alexpanin -- still needs code owner approval.

ddl-alexpanin

LGTM, please update CHANGELOG.md and can merge then.

…ecated and only use as fallback if id is provided

Co-authored-by: David Knupp <dknupp@users.noreply.github.com>

ddl-awroblicky requested a review from a team January 14, 2022 00:32

dknupp reviewed Jan 14, 2022

View reviewed changes

ddl-awroblicky marked this pull request as draft January 14, 2022 01:01

ddl-awroblicky marked this pull request as ready for review January 24, 2022 23:07

dknupp reviewed Jan 27, 2022

View reviewed changes

ddl-awroblicky force-pushed the fix_hardware_tier_name_override branch from 5cb68dd to 69ddbfc Compare January 28, 2022 09:01

ddl-awroblicky requested a review from dknupp February 9, 2022 18:40

dknupp reviewed Mar 1, 2022

View reviewed changes

ddl-alexpanin requested a review from dknupp March 8, 2022 17:47

dknupp approved these changes Mar 14, 2022

View reviewed changes

ddl-alexpanin approved these changes Mar 14, 2022

View reviewed changes

Andrew Wroblicky added 17 commits March 14, 2022 15:55

DOM-35464 convert passed in hardware tier name to hardware tier id

80d55d8

pr review code changes

12af863

remove unused method

9e9cf5c

for backwards compatibility allow hardware_tier_name but mark as depr…

aa28ddc

…ecated and only use as fallback if id is provided

add back hardware_tier_name validation

4b33a4e

add test

a1245f3

add missing colons

d9be112

add missing param to get_hardware_tier_id_from_name

46f1c1f

begin adding hardware tier insert/archive endpoints

5b626b4

more fixes

54c97de

test fixes

f476aba

more test fixes

1fd3da0

fix routes

b771961

more fixes

1d9ba98

add job_runtime_execution_details route

33220de

cleanup tests

66a07f2

minor fixes

81ac94c

Andrew Wroblicky and others added 7 commits March 14, 2022 15:55

fix description

916cf24

Update tests/test_jobs.py

377b67c

Co-authored-by: David Knupp <dknupp@users.noreply.github.com>

PEP-8 requires 2 lines between module-level functions

9c67cd5

Co-authored-by: David Knupp <dknupp@users.noreply.github.com>

pr review changes:

fe7be78

Add mocked endpoint to fix unit tests.

b36955b

Use a mock project id that looks less like the mock version number.

b084a33

update changelog

b7160d5

ddl-awroblicky force-pushed the fix_hardware_tier_name_override branch from 920b8f2 to b7160d5 Compare March 14, 2022 22:59

ddl-alexpanin merged commit 5e5ad64 into dominodatalab:master Mar 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOM-35464 convert passed in hardware tier name to hardware tier id #117

DOM-35464 convert passed in hardware tier name to hardware tier id #117

ddl-awroblicky commented Jan 14, 2022 •

edited

Loading

dknupp left a comment

dknupp Jan 14, 2022 •

edited

Loading

dknupp Jan 14, 2022

ddl-awroblicky Jan 14, 2022 •

edited

Loading

ddl-awroblicky Jan 24, 2022

dknupp Jan 24, 2022

dknupp commented Jan 26, 2022

dknupp Jan 27, 2022

dknupp Jan 27, 2022 •

edited

Loading

ddl-awroblicky Feb 7, 2022

dknupp Jan 27, 2022

dknupp Jan 27, 2022

dknupp Jan 27, 2022 •

edited

Loading

dknupp Jan 27, 2022 •

edited

Loading

dknupp left a comment

dknupp Mar 1, 2022

ddl-alexpanin Mar 14, 2022

dknupp Mar 1, 2022

dknupp Mar 1, 2022

dknupp Mar 1, 2022

dknupp Mar 1, 2022

dknupp left a comment

dknupp commented Mar 14, 2022

ddl-alexpanin left a comment

	resolved_hardware_tier_id = hardware_tier_id if hardware_tier_id is not None else self.get_hardware_tier_id_from_name(hardware_tier_name)
	resolved_hardware_tier_id = hardware_tier_id or self.get_hardware_tier_id_from_name(hardware_tier_name)

	non_default_hardware_tiers = (hardware_tier["isDefault"] == False for hardware_tier in hardware_tiers)
	non_default_hardware_tiers = [hwt for hwt in hardware_tiers if not hwt["isDefault"]]

	@pytest.mark.skipif(not domino_is_reachable(), reason="No access to a live Domino deployment")

	@pytest.mark.skipif(not domino_is_reachable(), reason="No access to a live Domino deployment")

	# deprecated but ensuring it still works for now

	# deprecated but ensuring it still works for now

DOM-35464 convert passed in hardware tier name to hardware tier id #117

DOM-35464 convert passed in hardware tier name to hardware tier id #117

Conversation

ddl-awroblicky commented Jan 14, 2022 • edited Loading

dknupp left a comment

Choose a reason for hiding this comment

dknupp Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddl-awroblicky Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dknupp commented Jan 26, 2022

Choose a reason for hiding this comment

dknupp Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dknupp Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

dknupp Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

dknupp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dknupp left a comment

Choose a reason for hiding this comment

dknupp commented Mar 14, 2022

ddl-alexpanin left a comment

Choose a reason for hiding this comment

ddl-awroblicky commented Jan 14, 2022 •

edited

Loading

dknupp Jan 14, 2022 •

edited

Loading

ddl-awroblicky Jan 14, 2022 •

edited

Loading

dknupp Jan 27, 2022 •

edited

Loading

dknupp Jan 27, 2022 •

edited

Loading

dknupp Jan 27, 2022 •

edited

Loading