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

issue #205 bugfix #206

Merged
merged 2 commits into from
Jun 14, 2023
Merged

issue #205 bugfix #206

merged 2 commits into from
Jun 14, 2023

Conversation

MaciejKarasek
Copy link
Contributor

Added if statements to check if value of attribute is not None. Function hasattr returns True when atrribute exists even if its value is None.

Related to issue #205

@NanoCode012
Copy link
Collaborator

Hello, thanks for the PR!

I'm curious on the effect of hasattr(config, "max_seq_len"). Would it work without this since we're performing the check you have newly added?

@MaciejKarasek
Copy link
Contributor Author

I'm curious on the effect of hasattr(config, "max_seq_len"). Would it work without this since we're performing the check you have newly added?

It would work if we are sure that attribute like max_seq_len is always a part of the config class. In case there is no attribute like that and if statement would look like this if config.max_seq_len: there's gonna be an error AttributeError: type object 'config' has no attribute 'max_seq_len'

@NanoCode012
Copy link
Collaborator

Ah, I made the mistake. Thank you for clarifying. I confused config with cfg. I thought they were the same.

@MaciejKarasek
Copy link
Contributor Author

Seems I forgot to install pre-commit, but i think it not gonna work on mine python 3.11 cause in .pre-commit-config.yaml there is line python: python3.9 so anyway :/

@NanoCode012
Copy link
Collaborator

NanoCode012 commented Jun 14, 2023

Hey @MaciejKarasek , no worries. Actually, my python (for local dev) is also 3.11, so it might work as well :)

However, if you do not want to install, that's fine also! Feel free to look at the CI Details, and it would show you the issue and maybe a potential fix.

Copy link
Collaborator

@winglian winglian left a comment

Choose a reason for hiding this comment

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

thank you! lmk if you need any help with the pre-commit hooks

@MaciejKarasek
Copy link
Contributor Author

Thanks guys

@MaciejKarasek
Copy link
Contributor Author

Should be all right now.

@winglian winglian merged commit 6f84980 into axolotl-ai-cloud:main Jun 14, 2023
mkeoliya pushed a commit to mkeoliya/axolotl that referenced this pull request Dec 15, 2023
djsaunde pushed a commit that referenced this pull request Dec 17, 2024
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.

3 participants