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

[BUG] Created Time and Last Updated Time are not implemented on Connector #2890

Closed
dbwiddis opened this issue Sep 4, 2024 · 1 comment · Fixed by #2922
Closed

[BUG] Created Time and Last Updated Time are not implemented on Connector #2890

dbwiddis opened this issue Sep 4, 2024 · 1 comment · Fixed by #2922
Assignees
Labels
bug Something isn't working good first issue Good for newcomers v2.18.0 Issues targeting release v2.18.0

Comments

@dbwiddis
Copy link
Member

dbwiddis commented Sep 4, 2024

What is the bug?

The .plugin-ml-connector index contains field mappings for created_time and last_updated_time but these are never populated.

How can one reproduce the bug?
Steps to reproduce the behavior:

  1. Create a connector.
  2. Inspect the created document using the REST API
  3. Observe there are no created_time or last_updated_time fields.

What is the expected behavior?

  • the created and last updated time are both populated with the same Instant.now() upon instantiation, or at least set before index request
  • the last updated time is updated before an update request

What is your host/environment?

OpenSearch :)

Do you have any screenshots?

http://localhost:9200/.plugins-ml-connector/_doc/44ymvpEBPIzofVXwNZHj
{
    "_index": ".plugins-ml-connector",
    "_id": "44ymvpEBPIzofVXwNZHj",
    "_version": 1,
    "_seq_no": 0,
    "_primary_term": 1,
    "found": true,
    "_source": {
        "name": "Cohere Chat Model",
        "version": "1",
        "description": "The connector to Cohere's public chat API",
        "protocol": "http",
        "parameters": {
            "model": "command"
        },
        "credential": {
            "cohere_key": "REDACTED"
        },
        "actions": [
            {
                "action_type": "PREDICT",
                "method": "POST",
                "url": "https://api.cohere.ai/v1/chat",
                "headers": {
                    "Authorization": "Bearer ${credential.cohere_key}",
                    "Request-Source": "unspecified:opensearch"
                },
                "request_body": "{ \"message\": \"${parameters.inputs}\", \"model\": \"${parameters.model}\" }"
            }
        ]
    }
}

Do you have any additional context?

The HttpConnector class has fields for createdTime and lastUpdatedTime which are properly parsed from incoming transport or XContent, and properly sent to outgoing transport or XContent unless null. The problem is that they are always null.

  • there are no setters for them
  • they are never given any values

Suggested fix(es):

  • in the AbstractConnector, assign them a value on instantiating. (Use the same value for both.)
  • Add a setter for the updated time
  • Set the updated time when using the UpdateConnector API.
@dbwiddis dbwiddis added bug Something isn't working good first issue Good for newcomers untriaged labels Sep 4, 2024
@brianf-aws
Copy link
Contributor

Can I get assigned this bug?

@dhrubo-os dhrubo-os moved this to In Progress in ml-commons projects Sep 6, 2024
@dhrubo-os dhrubo-os added the v2.18.0 Issues targeting release v2.18.0 label Sep 6, 2024
brianf-aws added a commit to brianf-aws/ml-commons that referenced this issue Sep 10, 2024
fixes opensearch-project#2890 Currently any class that extends the AbstractConnector class has the fields createdTime and lastUpdatedTime set to null. The solution was instantiating the fields in the constructor of the AbstractConnector class, as well updating it within the HTTPConnector class whenever an update happens. Many tests were modified to catch the time fields being populated as such there will be many differences on the string in order to get around the timing issue when doing tests.

Signed-off-by: Brian Flores <[email protected]>
brianf-aws added a commit to brianf-aws/ml-commons that referenced this issue Sep 13, 2024
fixes opensearch-project#2890 Currently any class that extends the AbstractConnector class has the fields createdTime and lastUpdatedTime set to null. The solution was instantiating the fields in the constructor of the AbstractConnector class, as well updating it within the HTTPConnector class whenever an update happens. Many tests were modified to catch the time fields being populated as such there will be many differences on the string in order to get around the timing issue when doing tests.

Signed-off-by: Brian Flores <[email protected]>
brianf-aws added a commit to brianf-aws/ml-commons that referenced this issue Sep 17, 2024
fixes opensearch-project#2890 Currently any class that extends the AbstractConnector class has the fields createdTime and lastUpdatedTime set to null. The solution was instantiating the fields in the constructor of the AbstractConnector class, as well updating it within the HTTPConnector class whenever an update happens. Many tests were modified to catch the time fields being populated as such there will be many differences on the string in order to get around the timing issue when doing tests.

Signed-off-by: Brian Flores <[email protected]>
brianf-aws added a commit to brianf-aws/ml-commons that referenced this issue Sep 17, 2024
fixes opensearch-project#2890 when applying a code change like this previous connectors would have a weird bug where upon calling GET on them would change the timestamp. In this commit, it remains the old connectors without time fields while new ones have time fields, newer connectors will have correct and updated Timestamp. Manual testing was done on a local cluster and two unit tests were done to inspect the time changes on creation and update
brianf-aws added a commit to brianf-aws/ml-commons that referenced this issue Sep 17, 2024
fixes opensearch-project#2890 when applying a code change like this previous connectors would have a weird bug where upon calling GET on them would change the timestamp. In this commit, it remains the old connectors without time fields while new ones have time fields, newer connectors will have correct and updated Timestamp. Manual testing was done on a local cluster and two unit tests were done to inspect the time changes on creation and update
brianf-aws added a commit to brianf-aws/ml-commons that referenced this issue Sep 17, 2024
fixes opensearch-project#2890 when applying a code change like this previous connectors would have a weird bug where upon calling GET on them would change the timestamp. In this commit, it remains the old connectors without time fields while new ones have time fields, newer connectors will have correct and updated Timestamp. Manual testing was done on a local cluster and two unit tests were done to inspect the time changes on creation and update

Signed-off-by: Brian Flores <[email protected]>
@github-project-automation github-project-automation bot moved this from In Progress to Done in ml-commons projects Oct 1, 2024
brianf-aws added a commit to brianf-aws/ml-commons that referenced this issue Oct 2, 2024
* populate time fields for connectors on return

fixes opensearch-project#2890 Currently any class that extends the AbstractConnector class has the fields createdTime and lastUpdatedTime set to null. The solution was instantiating the fields in the constructor of the AbstractConnector class, as well updating it within the HTTPConnector class whenever an update happens. Many tests were modified to catch the time fields being populated as such there will be many differences on the string in order to get around the timing issue when doing tests.

Signed-off-by: Brian Flores <[email protected]>

* fixes backward compatability issues with old connectors

fixes opensearch-project#2890 when applying a code change like this previous connectors would have a weird bug where upon calling GET on them would change the timestamp. In this commit, it remains the old connectors without time fields while new ones have time fields, newer connectors will have correct and updated Timestamp. Manual testing was done on a local cluster and two unit tests were done to inspect the time changes on creation and update

Signed-off-by: Brian Flores <[email protected]>

* fix failing MLRegisterModelInutTest.testToXContent tests

Originally this commit was cherry picked from the 2.x branch and as such code changes affected the new build that werent caught on the previous commit 8c006de. Reformatted tests that were failing as the behavior implemented in previous commits was to not display time fields if a connector does not have them in the first place. gradlew build was done to assure the tests passed

Signed-off-by: Brian Flores <[email protected]>

* Reverts back model tests that were modified incorrectly by connector change

When creating a code change to the connector it propagated the new change of the object that affected many UTs, but after changing the logic of indexing the new connector, change the old changes for the unit test involving models with connectors had to be reverted back. UTs specifically for the indexed connectors have been created in UpdateConnectorTransportActionTests were done to capture this

Signed-off-by: Brian Flores <[email protected]>

* Adds lastUpdateTime to Old Connectors

Previoulsy we didnt consider the old connectors to have time fields at all, But given offline discussion if we add time fields to old connectors users could get more information moving forward without breaking any backward features. The solution to this was setting the last updated time in the update connector api; now moving forward any connector gets attached a last updated time field. I updated the testUpdateConnectorDoesNotUpdateHTTPCOnnectorTimeFields method to check that lastUpdateTime has a timestamp but that createdTime has no time field.

Signed-off-by: Brian Flores <[email protected]>

* Fixes wildcard import in UpdateConnectorTransportActionTests

Signed-off-by: Brian Flores <[email protected]>

---------

Signed-off-by: Brian Flores <[email protected]>
(cherry picked from commit 88eaefd)
dhrubo-os pushed a commit that referenced this issue Oct 3, 2024
* populate time fields for connectors on return

fixes #2890 Currently any class that extends the AbstractConnector class has the fields createdTime and lastUpdatedTime set to null. The solution was instantiating the fields in the constructor of the AbstractConnector class, as well updating it within the HTTPConnector class whenever an update happens. Many tests were modified to catch the time fields being populated as such there will be many differences on the string in order to get around the timing issue when doing tests.

Signed-off-by: Brian Flores <[email protected]>

* fixes backward compatability issues with old connectors

fixes #2890 when applying a code change like this previous connectors would have a weird bug where upon calling GET on them would change the timestamp. In this commit, it remains the old connectors without time fields while new ones have time fields, newer connectors will have correct and updated Timestamp. Manual testing was done on a local cluster and two unit tests were done to inspect the time changes on creation and update

Signed-off-by: Brian Flores <[email protected]>

* fix failing MLRegisterModelInutTest.testToXContent tests

Originally this commit was cherry picked from the 2.x branch and as such code changes affected the new build that werent caught on the previous commit 8c006de. Reformatted tests that were failing as the behavior implemented in previous commits was to not display time fields if a connector does not have them in the first place. gradlew build was done to assure the tests passed

Signed-off-by: Brian Flores <[email protected]>

* Reverts back model tests that were modified incorrectly by connector change

When creating a code change to the connector it propagated the new change of the object that affected many UTs, but after changing the logic of indexing the new connector, change the old changes for the unit test involving models with connectors had to be reverted back. UTs specifically for the indexed connectors have been created in UpdateConnectorTransportActionTests were done to capture this

Signed-off-by: Brian Flores <[email protected]>

* Adds lastUpdateTime to Old Connectors

Previoulsy we didnt consider the old connectors to have time fields at all, But given offline discussion if we add time fields to old connectors users could get more information moving forward without breaking any backward features. The solution to this was setting the last updated time in the update connector api; now moving forward any connector gets attached a last updated time field. I updated the testUpdateConnectorDoesNotUpdateHTTPCOnnectorTimeFields method to check that lastUpdateTime has a timestamp but that createdTime has no time field.

Signed-off-by: Brian Flores <[email protected]>

* Fixes wildcard import in UpdateConnectorTransportActionTests

Signed-off-by: Brian Flores <[email protected]>

---------

Signed-off-by: Brian Flores <[email protected]>
(cherry picked from commit 88eaefd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers v2.18.0 Issues targeting release v2.18.0
Projects
3 participants