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

added setup.py #10

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

Agent-Hellboy
Copy link

The name dsm is already taken so please choose another name or add suffix or prefix to it.
Also, add keywords.
if you allow then I can also upload it to the PyPi.
please review and provide suggestions.

@chiragnagpal chiragnagpal linked an issue Nov 1, 2020 that may be closed by this pull request
@chiragnagpal
Copy link
Collaborator

chiragnagpal commented Nov 1, 2020

Hi @Agent-Hellboy thanks for the PR, good job 👍

two quick points

  • I think it would be better for me to also be listed as maintainer for now in setup.py
  • instead of a pypi package, can you try to run "pip install <git repo url>" and confirm that that works?

If yes then:

  • change .travis.yml to execute that during testing (instead of running "pip install -r requirements.txt")
  • add a section on installing the package using pip in the readme.md

Feel free to update the same PR with the changes.

@Agent-Hellboy
Copy link
Author

@chiragnagpal please review.

@Agent-Hellboy
Copy link
Author

Sir, Currently, I have added the dependencies in install_requires filed of setup.py file explicitly. But if the dependencies increase then it may breaks.
If this is the only required dependencies then this is fine but if dependencies increases in the future then we have to parse the requirements.txt file. So that we could avoid changing setup.py file frequently.

@chiragnagpal chiragnagpal reopened this Nov 1, 2020
@Agent-Hellboy
Copy link
Author

Agent-Hellboy commented Nov 2, 2020

sorry about this.

I found this from https://packaging.python.org/tutorials/installing-packages/
Source Distributions vs Wheels
pip can install from either Source Distributions (sdist) or Wheels, but if both are present on PyPI, pip will prefer a compatible wheel.
Wheels are a pre-built distribution format that provides faster installation compared to Source Distributions (sdist), especially when a project contains compiled extensions.
If pip does not find a wheel to install, it will locally build a wheel and cache it for future installs, instead of rebuilding the source distribution in the future.

pip install repo_link is working currently.

And about adding this to the CI pipeline. if I add this to.travis.yml I am not sure but it will only fetch dependencies from the master branch or we have to specify that info in every branch .travic.yml file. I thick pip install. will be a better choice for CI.
Sorry about adding the pip install. instead of pip install repo_link in README.
If you allow I will remove my name from maintainer and also update the README and about CI I need some clarification.

About adding wheel to dependency I think if you have pip then you already had wheel

found this from pyproject.toml file of pip

[build-system]
requires = ["setuptools", "wheel"]
build-backend = "setuptools.build_meta" 

again sorry about adding pip install wheel to .travis.yml file it was there when I think of installing the package using python setup.py install
I have tested pip install repo_link it is working perfectly fine for every branch.

@chiragnagpal
Copy link
Collaborator

@Agent-Hellboy No problem at all, I appreciate your help.

  • As of now we only care about Pip install git+<master_branch>; other branches are just for development purposes , so don't worry about them.
  • Feel free to update the current PR with the changes we discussed. we don't want wheel (as of now) just make sure pip install git+<git_url> works. (Since requirements.txt is already in the source you wouldn't need to separately run pip install -r requirements.txt)

@Agent-Hellboy
Copy link
Author

One more query.
I have tested only the load_datasets function of dsm.dataset. And it is working. Does this mean every other function will work?

>>> import dsm
>>> import dsm.datasets
>>>dsm.datasets.load_dataset()

I have also checked the get_data function of pkgutil form this path /usr/local/lib/python3.8/dist-packages

>>> import pkgutil
>>> data = pkgutil.get_data('dsm','datasets/support2.csv')

if name means /usr/local/lib/python3.8/dist-packages/dsm
then I think it will work.

@chiragnagpal
Copy link
Collaborator

Hmm, when you update the PR travis would automatically check for these functions, if something is wrong it travis would break somewhere....

@Agent-Hellboy
Copy link
Author

Yes, sir, Travis will check, But I have experience of breaking the code of a package having a subpackage or data file because I was running the test cases before build. That why I was hesitant. Now I get it. why you are insisting on pip install repo_link instead of pip install -r requirements.txt because for pip install repo_link Travis will run the unit test after the build.

@Agent-Hellboy
Copy link
Author

Agent-Hellboy commented Nov 2, 2020

Sir, there is no setup.py file in your repo that's why CI fails. either we have to add my repo link for this build to pass or we have to update the .travis file after this merge. I have taken the latter approach.

@Jesse-Islam
Copy link

Is there a reason why this pull request has not been merged? I'd like to add this package to my environment however the safeguards in place with a proper setup.py are important to many potential users, myself included.

@chiragnagpal
Copy link
Collaborator

@Jesse-Islam agreed. feel free to create a PR or restart the previous one.

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.

Make code a package
3 participants