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

Hap score - Example Notebook #840

Merged
merged 16 commits into from
Dec 7, 2024
Merged

Hap score - Example Notebook #840

merged 16 commits into from
Dec 7, 2024

Conversation

AishaDarga
Copy link
Contributor

Why are these changes needed?

Related issue number (if any).

@shahrokhDaijavad
Copy link
Member

@ian-cho This is another variation of a Notebook that uses your hap transform. Can you please review and provide comments?

@ian-cho
Copy link
Collaborator

ian-cho commented Dec 4, 2024

@shahrokhDaijavad As long as this modification does not affect other modules in the hap/python directory or make them incompatible with each other, it is fine with me.

@shahrokhDaijavad
Copy link
Member

Thanks, @ian-cho. So, just to confirm, you are ok with the changes made in hap_config.py and hap_local_python.py files, right? Any future changes to the hap transform directory need to be reviewed again.

@ian-cho
Copy link
Collaborator

ian-cho commented Dec 4, 2024

@shahrokhDaijavad Thanks for asking. If the changes affect other modules, would you mind helping resolve them when I am not available? Previously, @touma-I and you helped a lot in ensuring module compatible with each other I remember, and it was not easy for me to ensure that.

Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
@AishaDarga
Copy link
Contributor Author

AishaDarga commented Dec 4, 2024

@shahrokhDaijavad @ian-cho I've updated the notebook to ensure compatibility with Google Colab and included the installation of all necessary packages directly within the notebook. Could you please review it?
Thank you!

@shahrokhDaijavad
Copy link
Member

@AishaDarga Have you tested this notebook? I ran into errors, running both on the local machine and on the Google Colab.

On the local machine, I get errors about hap_params not being defined.
image
and
image

On Google Colab, I get the following error:
image

@AishaDarga
Copy link
Contributor Author

@shahrokhDaijavad Yes, I've tested the notebook thoroughly on my system. I'll try to re-install and test it afresh. Thank you!

AishaDarga and others added 2 commits December 5, 2024 09:27
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
@shahrokhDaijavad
Copy link
Member

@AishaDarga I just ran your latest notebook successfully, so it works fine. However, I understand you had some discussions with @touma-I about not changing the transform code. When you make the necessary changes, I will be happy to test again.

Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
@AishaDarga
Copy link
Contributor Author

AishaDarga commented Dec 6, 2024

@touma-I @shahrokhDaijavad Thank you for your feedback. I've made all necessary changes in the latest commit. This version of the code leverages the HAP transform as is, without making any changes. Could you please review it

Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
…book accordingly

Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Copy link
Member

@shahrokhDaijavad shahrokhDaijavad left a comment

Choose a reason for hiding this comment

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

This version, which I have tested successfully, looks good to me and the directories are also clean. Thanks, @AishaDarga.

Copy link
Collaborator

@touma-I touma-I 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 removing the parquet file from git since we can regenerate it from css. Thanks

Copy link
Collaborator

@touma-I touma-I Dec 6, 2024

Choose a reason for hiding this comment

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

remove data-prep-connector since you are NOT using it. This line can be deleted:

! pip install data-prep-connector

Copy link
Collaborator

@touma-I touma-I Dec 6, 2024

Choose a reason for hiding this comment

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

You do not need this line. Those are transitive dependencies of the transform itself and could break the notebook in the future if they are changed by new releases of the ransform.

! pip install nltk==3.9.1 transformers==4.38.2 torch>=2.2.2,<=2.4.1 pandas==2.2.2

Copy link
Member

Choose a reason for hiding this comment

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

Adding these is my fault, @touma-I! Since I could get the notebook to work only after pip installing requirements.txt, I thought they were needed, but it turns out my problem was something else! In any case, what you are saying is that these dependencies are taken care of by the pip install of the transform itself and should not be done here.

Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
Signed-off-by: Aisha Mohammed Farooq Darga <[email protected]>
@AishaDarga
Copy link
Contributor Author

@touma-I @shahrokhDaijavad I've made all necessary changes and addressed the comments given above. I've tested the latest changes and it works fine for me. Could please review it?
image

Copy link
Member

@shahrokhDaijavad shahrokhDaijavad left a comment

Choose a reason for hiding this comment

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

Nice job, @AishaDarga.
@touma-I I have tested the latest version successfully with all the changes we had asked. I recommend merging this.

@touma-I touma-I merged commit 2e2e5d4 into IBM:dev Dec 7, 2024
1 check 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