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

Update to Support COMET API Changes #217

Merged

Conversation

Click2cloud-Abhijeet
Copy link

Commit Description:
This commit updates the FarmVibes codebase to align with the recent changes in the COMET API requirements:

New Endpoint:
Updated the API endpoint for submitting runs to /uploadapiqueue.

API Key Support:
Integrated support for including the apikey parameter in API requests, as now mandated by COMET API.

Code Adjustments:
Refactored vibe_lib package to incorporate the new endpoint and API key requirements.
Ensured backward compatibility and added error handling for scenarios where the API key is missing or invalid.

@Click2cloud-Abhijeet
Copy link
Author

@microsoft-github-policy-service agree company="Click2Cloud Inc"

@renatolfc
Copy link
Contributor

Hello @Click2cloud-Abhijeet, thanks for your contribution!

Can you please also update the https://github.com/microsoft/farmvibes-ai/blob/main/notebooks/carbon/whatif.ipynb Jupyter notebook so that it can run with this updated version of comet?

@rafaspadilha rafaspadilha requested a review from Copilot December 17, 2024 12:51
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/vibe_lib/vibe_lib/comet_farm/comet_server.py:29

  • Ensure that the new behavior introduced by the apiKey parameter is covered by tests.
apiKey: str

@rafaspadilha rafaspadilha requested a review from Copilot December 17, 2024 13:01
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • notebooks/carbon/scenario_seasonal_fields.json: Language not supported
Comments suppressed due to low confidence (2)

src/vibe_lib/vibe_lib/comet_farm/comet_server.py:29

  • [nitpick] The parameter apiKey should be renamed to api_key to follow the consistent naming convention used in the rest of the class.
apiKey: str

src/vibe_lib/vibe_lib/comet_farm/comet_server.py:69

  • Ensure that the new behavior introduced by the apiKey parameter and the new endpoint is covered by tests.
"apikey": self.comet_request.apiKey

@Click2cloud-Abhijeet
Copy link
Author

modified the code with Ruff test

@Click2cloud-Abhijeet
Copy link
Author

I have updated the code and run 'make local' then I tried to run the 'carbon sequestration.ipynb' file , but there I was getting an error.
notebook error

@brsilvarec
Copy link
Contributor

@Click2cloud-Abhijeet, I think you also need to configure the api_token in the file workflows/farm_ai/carbon_local/carbon_whatif.yaml.

I also suggest you rename the variable api_token to comet_api_token as we have other tokens in the repo.

@brsilvarec
Copy link
Contributor

@Click2cloud-Abhijeet, I think you also need to configure the api_token in the file workflows/farm_ai/carbon_local/carbon_whatif.yaml.

I also suggest you rename the variable api_token to comet_api_token as we have other tokens in the repo.

After making the suggested changes on my dev env, the code worked as expected.

image

@brsilvarec
Copy link
Contributor

This workflow workflows/farm_ai/carbon_local/admag_carbon_integration.yaml also depends on carbon and needs to update. I think we also need to revisit this notebook. notebooks/admag/azure_data_manager_for_agriculture_and_comet_farm_api_example.ipynb

@Click2cloud-Abhijeet
Copy link
Author

Click2cloud-Abhijeet commented Dec 27, 2024

@Click2cloud-Abhijeet, I think you also need to configure the api_token in the file workflows/farm_ai/carbon_local/carbon_whatif.yaml.

I also suggest you rename the variable api_token to comet_api_token as we have other tokens in the repo.

As per suggestion I have rename the comet_api_key .

@Click2cloud-Abhijeet
Copy link
Author

@Click2cloud-Abhijeet, I think you also need to configure the api_token in the file workflows/farm_ai/carbon_local/carbon_whatif.yaml.

I also suggest you rename the variable api_token to comet_api_token as we have other tokens in the repo.

"comet_api_key" added in the given location workflows/farm_ai/carbon_local/carbon_whatif.yaml.

Copy link
Contributor

@brsilvarec brsilvarec left a comment

Choose a reason for hiding this comment

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

Please consider reviewing the minor comments. PR looks good to me

Copy link
Contributor

@rafaspadilha rafaspadilha left a comment

Choose a reason for hiding this comment

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

The PR is looking good. I've just added some minor changes before approving it. Thanks for all the work, folks!

Copy link
Collaborator

@v-ngangarapu v-ngangarapu left a comment

Choose a reason for hiding this comment

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

See the comments.

@rafaspadilha rafaspadilha merged commit 5b602b7 into microsoft:dev Feb 17, 2025
8 of 9 checks passed
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