-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Python Add the Amazon Neptune Python follow #7473
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you've made the high-level fixes and it matches the specification, I can do a full review.
@pytest.fixture(scope="module") | ||
def neptune_client(): | ||
"""Create a real Neptune boto3 client for integration testing.""" | ||
client = boto3.client("neptune", region_name="us-east-1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the current testing patterns in the library until we decide as a team to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modelled tests after the Keyspace. Confirmed we follow pattern too
The Hello and Scenario examples run as excepted. The tests have been refactored to follow other Python tests such as Keyspace. The Spec was updated to include SME contributed code examples. |
print(f"Deleting DB Cluster: {cluster_id}") | ||
neptune_client.delete_db_cluster(**request) | ||
except ClientError as e: | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the exception handling for the ResourceNotFoundException?
try: | ||
db_cluster_id = create_db_cluster(neptune_client, cluster_name) | ||
except ClientError as ce: | ||
code = ce.response["Error"]["Code"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception handling should be in the function, not in the scenario, so that it is visible within the code snippet. Same for all handling, it should follow the pattern of being in the function with the client call.
Main entry point: create NeptuneGraph client and call graph creation. | ||
""" | ||
# Hypothetical client - boto3 currently doesn't have NeptuneGraph client, so replace with actual client if available | ||
neptune_graph_client = boto3.client("neptune") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hypothetical client? This seems incorrect, either there is a client for this code or there isn't. The code below seems to correctly create a neptune-graph
client, so why is this one hypothetical?
try: | ||
print("Creating Neptune graph...") | ||
|
||
# Hypothetical method for create_graph, adjust accordingly if you use HTTP API or SDK extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the method hypothetical?
|
||
try: | ||
response = client.execute_query( | ||
GraphIdentifier=graph_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The capitalization in the parameters of all of these methods does not match the documentation and it also does not match the code provided by the SME:
`resp =client.execute_query(
graphIdentifier = graph_id,
queryString="MATCH (n {code: 'ANC'}) RETURN n",
language='OPEN_CYPHER'
)
print(resp['payload'].read().decode('UTF-8')`
Please recheck that the parameters match exactly on all these graph and data examples.
|
||
# Import your scenario file; ensure Python can locate it. | ||
# If your file is named `neptune_scenario.py`, this import will work: | ||
import NeptuneScenario |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not able to run any of the tests that use the scenario file, I get an error: ModuleNotFoundError: No module named 'NeptuneScenario'
# --- Success case --- | ||
mock_neptune = MagicMock() | ||
paginator_mock = MagicMock() | ||
mock_neptune.get_paginator.return_value = paginator_mock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to use the established stubber pattern for the client responses, see the files in the test_tools
directory, and add a file such as neptune_stubber.py
for each client to stub the responses.
|
||
As there is limited room in aboce table, the metadata key for `executeOpenCypherExplainQuery`is neptune_ExecuteOpenCypherExplainQuery. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can go back into the table, it seems to look fine there and will be harder to find here.
This pull request adds the Amazon Neptune Python follow
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.