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

Fix python version in setup for numba errors #106

Closed
wants to merge 5 commits into from
Closed

Fix python version in setup for numba errors #106

wants to merge 5 commits into from

Conversation

sky-2002
Copy link

Description: As per the issue given here, numba requires python version <3.12 and >=3.8. According to the PEP 440 version specifiers, I have added <3.12 in the setup function in setup.py file.

Note: The installation will still have the issue, but now the setup will have python version requirements so users can change versions accordingly.

@ddkang
Copy link
Owner

ddkang commented Nov 18, 2023

Thanks for putting this together, this would be our first external contribution!

I think since numba is our only 3.12 blocker, we should actually make numba optional and jit the functions on demand, if it's available. Do you think you could do that instead?

@sky-2002
Copy link
Author

Hi @ddkang, I could see numba being used only in one function, so I have tried making it optional (checking with import). Let me know if that works.

dists = np.sqrt(np.sum((x - embeddings[i]) ** 2))
if dists < min_dists[i]:
min_dists[i] = dists
_get_and_update_dists_no_numba(x, embeddings, min_dists)
Copy link
Owner

Choose a reason for hiding this comment

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

I believe you can just define the function and move the shared code out of the try/except block

Copy link
Author

Choose a reason for hiding this comment

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

Hi @ddkang , moved shared code outside, let me know if you need any changes.

Copy link
Owner

Choose a reason for hiding this comment

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

I meant you can simply share the function name and call the function outside of the try/except block

Copy link
Author

Choose a reason for hiding this comment

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

You mean like this? -

def get_and_update_dists(x: np.ndarray, embeddings: np.ndarray, min_dists: np.ndarray):
  '''
  :param x: embedding of cluster representatives
  :param embeddings: embeddings of all data
  :min_dists: array to record the minimum distance for each embedding to the embedding of cluster representatives
  '''
  try:
    from numba import njit, prange

    @njit(parallel=True)
    def _get_and_update_dists(x: np.ndarray, embeddings: np.ndarray, min_dists: np.ndarray):

      for i in prange(len(embeddings)):
        _get_and_update_dists_shared(x, i, min_dists, embeddings)

  except:
    def _get_and_update_dists(x: np.ndarray, embeddings: np.ndarray, min_dists: np.ndarray):
      for i in range(len(embeddings)):
        _get_and_update_dists_shared(x, i, min_dists, embeddings)
  
  _get_and_update_dists(x, embeddings, min_dists)

Copy link
Owner

Choose a reason for hiding this comment

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

yes

Copy link
Author

Choose a reason for hiding this comment

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

Okay, pushed the changes.

@ddkang ddkang requested a review from ttt-77 November 18, 2023 18:50
@ddkang
Copy link
Owner

ddkang commented Nov 18, 2023

@ttt-77 can you review these changes?

from typing import Optional

from aidb.vector_database.vector_database_config import TastiConfig

@njit(parallel=True)
def _get_and_update_dists_shared(x, i, min_dists, embeddings):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to move it into function 'get_and_update_dists'.

Copy link
Author

Choose a reason for hiding this comment

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

Okay @ttt-77 , moved it.

@ttt-77
Copy link
Collaborator

ttt-77 commented Nov 18, 2023

The code logic looks good to me. Could you test it with numba and without numba if possible?

@sky-2002
Copy link
Author

The code logic looks good to me. Could you test it with numba and without numba if possible?

Sure. One thing I wanted to ask, as numba is still there in requirements, users would still get the python version related error due to numba installation. Do we need to also make changes to requirements or what?

@ddkang
Copy link
Owner

ddkang commented Nov 19, 2023

Can we make the requirement optional? Like aidb[all] vs aidb[base] or something

@sky-2002
Copy link
Author

@ddkang Sorry for a late reply, cant we just add numba[some_name] in requirements instead of just numba. So that it only gets installed when someone does - pip install aidb[some_name]. Will that be good?

@ddkang
Copy link
Owner

ddkang commented Nov 21, 2023

Sure sounds good

@sky-2002
Copy link
Author

sky-2002 commented Nov 21, 2023

But I think aidb cannot be installed as pip install aidb as of now. So maybe temporarily, we can go with the method given here, which says that we can specify python version after the package: for example - numba; python_version=='3.11' in the requirements. Should I add this and push?

@sky-2002
Copy link
Author

@ddkang Do you also have a slack or discord channel for aidb, I am planning to add a new vectorDB to aidb(written code already), so I may need to ask doubts there.

@ddkang
Copy link
Owner

ddkang commented Nov 21, 2023

Hi @sky-2002 could you email me for the slack invite link? Thank you for helping with the development!

Sounds good regarding the temporary solution.

@ttt-77
Copy link
Collaborator

ttt-77 commented Nov 22, 2023

But I think aidb cannot be installed as pip install aidb as of now. So maybe temporarily, we can go with the method given here, which says that we can specify python version after the package: for example - numba; python_version=='3.11' in the requirements. Should I add this and push?

Try pip install ai-db

@sky-2002 sky-2002 closed this by deleting the head repository May 27, 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