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

feature: [VRD-679] New client function get_kafka_topics() #3727

Merged
merged 22 commits into from Apr 14, 2023

Conversation

ewagner-verta
Copy link
Contributor

@ewagner-verta ewagner-verta commented Apr 5, 2023

Impact and Context

  • Adds a new function to verta.client.Client that will allow users to fetch a list of available Kafka topics.
  • Updates KafkaSettings class to make the cluster_config_id argument optional.
  • Refactors verta._endpoint.Endpoint() to fetch the Kafka config ID when it is not included.
  • Adds property/unit tests to verify behavior with and without cluster_config_id.
  • Adds property/unit tests for new client function.
  • Includes some minor black formatting changes to bring changed files up to standard.

Risks and Area of Effect

Testing

  • Unit test
  • test_endpoint.py

Screen Shot 2023-04-05 at 12 07 16 PM

  • test_client_get_kafka_topics.py

Screen Shot 2023-04-05 at 12 09 16 PM

  • Deployed to dev env
  • Other (explain)
    Validation script for running against a real env with a Kafka config:
from verta import Client
from verta.endpoint import KafkaSettings
from verta.environment import Python
from verta.registry import verify_io, VertaModelBase


class Model(VertaModelBase):
    def __init__(self, artifacts=None):
        pass

    @verify_io
    def predict(self, input):
        return input


def main():
    client = Client()

    model_ver = client.get_or_create_registered_model(
        f"Kafka Model Testing"
    ).create_standard_model(
        Model,
        code_dependencies=[],
        environment=Python([]),
    )

    kafka_settings = KafkaSettings(
        input_topic="inputTopic",
        output_topic="outputTopic",
        error_topic="errorTopic",
    )
    assert kafka_settings._cluster_config_id is None
    print(f"\nKafka Settings Provided\n----------------------\n{kafka_settings}\n")

    client.create_endpoint("kafka-model-testing", kafka_settings=kafka_settings)
    endpoint = client.get_endpoint("kafka-model-testing")
    assert endpoint.kafka_settings.cluster_config_id is not None

    print(f"\nFinal Kafka Settings\n--------------------\n{endpoint.kafka_settings}\n")

if __name__ == "__main__":
    main()

Output:

got VERTA_HOST from environment
got VERTA_EMAIL from environment
got VERTA_DEV_KEY from environment
connection successfully established
created new RegisteredModel: Kafka Model Testing in workspace: ewagner
created new ModelVersion: ModelVersion 421771680719193545579
uploading model to Registry
upload complete
uploading model_api.json to Registry
upload complete
uploading custom_modules to Registry
upload complete

Kafka Settings Provided
----------------------
KafkaSettings('inputTopic', 'outputTopic', 'errorTopic', None)

Final Kafka Settings
--------------------
KafkaSettings('inputTopic', 'outputTopic', 'errorTopic', 'd5a8b0b8-c4ab-4d75-97e7-ecddc494402f')

Note the cluster_config_id (4th arg) has been fetched.

Reverting

  • Contains Migration - Do Not Revert

client/verta/tests/unit_tests/strategies.py Show resolved Hide resolved
client/verta/verta/endpoint/_kafka_settings.py Outdated Show resolved Hide resolved
client/verta/verta/endpoint/_kafka_settings.py Outdated Show resolved Hide resolved
client/verta/verta/endpoint/_endpoint.py Outdated Show resolved Hide resolved
client/verta/tests/unit_tests/conftest.py Outdated Show resolved Hide resolved
client/verta/tests/unit_tests/test_endpoint.py Outdated Show resolved Hide resolved
@ewagner-verta
Copy link
Contributor Author

!rebuild

Copy link
Contributor

@liuverta liuverta left a comment

Choose a reason for hiding this comment

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

@ewagner-verta
Copy link
Contributor Author

@liuverta Did you happen to see my prior responses in each of the two threads you referenced?

In the first case, using assume to ensure uniqueness in a list as suggested is failing for me, so I'm asking if you can make a specific code suggestion there or can we leave it working as is?

In the second case, sending a None value to the back-end would result in a 500 error that I think is ok to pass along as is. Again, if you have a specific suggestion or vision for that process, can you share?

client/verta/verta/client.py Show resolved Hide resolved
client/verta/tests/unit_tests/conftest.py Show resolved Hide resolved
client/verta/tests/unit_tests/test_endpoint.py Outdated Show resolved Hide resolved
client/verta/verta/endpoint/_endpoint.py Outdated Show resolved Hide resolved
client/verta/verta/endpoint/_endpoint.py Outdated Show resolved Hide resolved
client/verta/verta/endpoint/_kafka_settings.py Outdated Show resolved Hide resolved
client/verta/verta/endpoint/_kafka_settings.py Outdated Show resolved Hide resolved
Copy link
Contributor

@liuverta liuverta 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 tackling the tests 😵‍💫 Some feedback on the error messages, then I think we'll be good

client/verta/verta/endpoint/_endpoint.py Outdated Show resolved Hide resolved
client/verta/verta/endpoint/_endpoint.py Outdated Show resolved Hide resolved
ewagner-verta and others added 3 commits April 14, 2023 10:29
Co-authored-by: Liu <96442646+liuverta@users.noreply.github.com>
Co-authored-by: Liu <96442646+liuverta@users.noreply.github.com>
@ewagner-verta
Copy link
Contributor Author

@liuverta - No problem. Thanks for the review. I accepted the changes to the error messages and I just pushed a commit to update the unit tests accordingly. Cheers

@ewagner-verta ewagner-verta merged commit b412056 into main Apr 14, 2023
7 checks passed
@ewagner-verta ewagner-verta deleted the VRD-679_get_kafka_topics_client_function branch April 14, 2023 21:01
liuverta added a commit that referenced this pull request Apr 18, 2023
* feature: add client function for fetching Kafka topics

* refactor: make `cluster_config_id` optional in KafkaSettings class, update doc-string

* refactor: make endpoint fetch Kafka `cluster_config_id` from active Kafka config by default when it is not present in KafkaSettings.

* refactor: move hypothesis strategy to strategies.py with all the others

* test: expand hypothesis strategies with some mocked kafka elements

* test: add fixtures for mocking a Client and a RegisteredModelVersion

* test: add property/unit tests for the changes to Endpoint

* test: add property/unit tests for `Client.get_kafka_topics()`

* fix: final newline

* docs: fix attribute descritpion

* docs: fix arg descritpion

* test: drop `ignore_conn_err` from mocked client test fixture

* test: make arbitrary id numbers more explicit for test clarity

* docs: use `versionchanged` instead of `versionadded`

* docs: Reword doc string for `cluster_config_id`

* refactor: raise client errors for missing kafka config or missing config id

* test: fix return from get_stages api to simulate correct response and drop unused call (was a result of no value for stage being found)

* test: add unit tests for both new error conditions (no valid kafka configs, and config returned is missing the ID we need)

* Update client/verta/verta/endpoint/_endpoint.py

Co-authored-by: Liu <96442646+liuverta@users.noreply.github.com>

* Update client/verta/verta/endpoint/_endpoint.py

Co-authored-by: Liu <96442646+liuverta@users.noreply.github.com>

* test: update unit tests for changes to error messaging

---------

Co-authored-by: Liu <96442646+liuverta@users.noreply.github.com>
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.

None yet

2 participants