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

pypi release with new package name #240

Closed
cancan101 opened this issue Jan 19, 2016 · 36 comments
Closed

pypi release with new package name #240

cancan101 opened this issue Jan 19, 2016 · 36 comments

Comments

@cancan101
Copy link
Contributor

Can we get a pypi release of pydicom with the new package name? The fact you cannot install from pypi with the new package name is causing issues like: patmun/pynetdicom_legacy#43.

@darcymason
Copy link
Member

Yes, I think I'm close to a release. I had been holding off while thinking through some major backwards -incompatible changes that really have to be done in v1.0, but I think I have a way through that now.

@bennyrowland
Copy link

Any update on when we might see a 1.0 release on PyPI

@darcymason
Copy link
Member

I'm glad you asked. I haven't had much time to work on this lately, and It's been a sliding target; I'm now thinking I would like to see the color images questions solved (e.g. #262, #263), and I'd like to introduce memmap solutions and remove the old deferred read code (e.g. #139), as I suspect that would be much cleaner if backwards compatibility is not required.

I'm open to the idea, however, of pushing out a pre-release if people need the big fixes and enhancements. The import name change to 'pydicom' would be in that, so it probably only makes sense if it is in the 1.0 series. Maybe a 1.0 alpha release, with some changes still to come before final 1.0 release? My understanding is that pip would by default ignore pre-release versions, but they could be installed if explicitly specified.

I'd be interested in people's thoughts.

@bennyrowland
Copy link

I am working with Python 3 (due to other dependencies) and so I believe require pydicom 1.0. For me personally it is not a problem to pull the latest repo version and install from local files, but pydicom is a dependency of my own toolbox, so for users who want to install that via pip without having pydicom previously installed they end up with an older version and lots of issues, particularly because of the import name change. Documentation can go some way to addressing this issue but it would be nice to keep the installation path as simple as possible. I don't know how easy it is to force pip to install a pre-release version of a dependency but without that I will wait for the official release. Keep up the great work, anyway.

@norok2
Copy link

norok2 commented Sep 2, 2016

If you plan to support two versions of the library, or simply if you want to leave a stable or legacy version of it in PyPI, I would suggest you to go for name change in the PyPI package for the newer version. Something like pydicom2, would probably work. Otherwise, I would suggest to go for a deprecation warning / grace period.
I would like to see the newer version of the library in PyPI as soon as possible, as I also have a toolkit that does depend on your wonderful (many thanks!) package, and I would benefit from the changes you have done so far, and so would the potential users.

@darcymason
Copy link
Member

@norok2 I appreciate the suggestion of using name pydicom2, but I don't want to again make the mistake of having the library name and the import line be different. I know there will be confusion for a while, but hopefully people will quickly switch over to pydicom 1.0. For the most part, it is easy to continue using old code by simply changing import dicom to:

try:
   import pydicom as dicom
except ImportError:
   import dicom

This won't always be enough, as there are a few more library name changes, but for many uses, the most common line ds = dicom.read_file(...) will continue to work this way.

I will endeavor to provide instruction for installing the legacy pydicom when the new version is released. Pip can be instructed to get a specific version.

I'll be working on the pre-release for pydicom 1.0 this week. No promises, as there are some very thorny issues to work out around the pixel data handling for the new compressed image handlers. But I will do my best to get something out.

@norok2
Copy link

norok2 commented Sep 6, 2016

@darcymason actually, if that is what you are struggling with, why not publishing the old API as simply 'dicom' in PyPI i.e. changing the old name.
This will reflect in packages needing to update either their setup.py (but not requirements.txt, if that was correctly handled), which is probably a quite minimal change, or their implementation.
Most importantly, scripts will not need to be changed, not even the ImportError trick.
Only the environments needs to be changed.

Maybe, it might be worth considering adding a warning of this API change during the import ASAP, something like:

PyDICOM will undergo some API change with the release of version 1.0 (expected by <DATE>).
The current API will still be available in its current state with the `DICOM` PyPI package.
Please change your environment accordingly.

What do you think?

@darcymason
Copy link
Member

@norok2, I'm open to the idea, but I'm not clear on how it is supposed to work. For your comment:

This will reflect in packages needing to update either their setup.py (but not requirements.txt, if that was correctly handled)...

Was that meant to be 'setup.py OR requirements.txt'? It seems to me it could be either one needing updating.

Let's take a concrete example. Let's say a package has a requirements,txt with pydicom >= 0.9.7 in it. It seems to me that could simply be changed to pydicom = 0.9.9 and the package would then have no problems with my proposal. Under your proposal, I suppose it could be changed to dicom >= 0.9.9 and that would work. The latter would allow for bug updates to the deprecated 'dicom' package, but I don't plan on doing any anyway. I would think it would be better to take advantage of the new pydicom features and bug fixes; in its next release, the package could just change a few lines to adapt to pydicom 1.0, and then change the requirements.txt for new releases to pydicom >= 1.0.0

No matter what things are called, the old import dicom will continue to work if an old version has already been installed. It can exist alongside the new pydicom package name. For new installs, any package will have to make an edit to their requirements.txt or setup.py, unless an exact version number was specified (or a '<='), in which case no change would be needed.

Does that seem right, or am I missing something? These are very important issues and I would like to choose the path that causes the least confusion.

@norok2
Copy link

norok2 commented Sep 6, 2016

If a new version of pydicom pops up without creating a dicom alias to the old version (which is, by the way, not much time-consuming at all, if needed I can do that myself), updating the package with pip will cause the old version to be unavailable within that environment.
It would probably be still possible to install both versions, but not as cleanly as it would be with my suggestion. By creating the dicom package, it would be possible to have installed and managed by pip both versions, so that packages depending on the old API may coexist with packages depending on the new API. This does not need to be a permanent solution, nor something that you would need to maintain, but rather a way to give your user-base a way of smoothly migrating to the new API when it suits them better. The main issue is that if version 1.0.0 is actually deemed to be stable, the API change should not hold it back from finding its way to pip as it seems that your user-base is willing to move to the newer API, but the unavailability to it in pip is holding back the distribution of packages based on it.

If one is in the need of using the ImportError trick, but it is not really affected by the API change, then it would be far better to refactor the code to use the pydicom namespace, and explicitly access the newer version (it is relatively easy and safe to do so with moder IDEs), and there will not need to further edit the environment beyond updating pydicom.

The requirements.txt should have been produced by a run of pip freeze (because it is meant to be used for the purpose of results reproducibility in local scripts), in which case you would have gotten an exact version matching e.g. pydicom==0.9.9 and this would give you no problem whatsoever (unless you decide to remove the egg from PyPI, which of course you should not).
The setup.py is used instead to manage a package dependency for distribution and there your package should appear as pydicom if it is not affected by the API change, while should be changed to pydicom >= 1.0.0 to enforce the use of the new API, or to pydicom <= 0.9.9 to use the old API , the latter being likely to cause dependency issues if two packages in the same environment require mutually exclusive versions. With my suggestions one would be able to list the dependency simply as dicom and then all dependency issues would be avoided.

@darcymason
Copy link
Member

Okay, it took me a while to absorb this, but I see the logic. Thanks for that. It is simple enough to add the extra package on PyPI. I also like the idea of the warning about the API change (more information for people is always better), so I can release a new 0.9.9.1 or similar name, with a warning added, so that will become the default install until 1.0 comes out. I'll send a notice out on the pydicom google group as well.

@norok2
Copy link

norok2 commented Sep 7, 2016

No problem. Also, the 0.9.9.1 version (the one living in the dicom PyPI package) should have a warning on import so that people willing to use the newer version are being made aware in the future. Something like:

This package is based on an older version of PyDICOM and is no longer maintained (as of <DATE>).
You can access the new PyDICOM features and API by installing `pydicom` from PyPI.

@azmeuk
Copy link
Contributor

azmeuk commented Oct 4, 2016

Hi. Is there some news about this topic?

@darcymason
Copy link
Member

I've done some of the necessary work in the features/image-handlers branch, but unfortunately have not managed to spend much time on this lately. I think I could leave that feature with gdcm only enabled, and release with that to start. But I'd like to put a little more testing into place on it.
Can't really commit to a timeline, though, but I will do my best.

@ZeroCool2u
Copy link

Hi @darcymason don't mean to bother you, just wondering if there's been any further update on this. Thanks for all the hard work!

@darcymason
Copy link
Member

I'm glad you asked, I've been meaning to give an update as I know I've gone quiet for too long.

I had been struggling with how to deal with a number of things, but I've finally decided to mostly defer those decisions and get on with releasing the alpha. A few days ago I had just started to work on the release of "dicom" as its own project as previously discussed, and then, unfortunately, got hit with a horrible virus (human, not computer virus) and have been knocked on my back the last few days. But once I've recovered some more, I will put that in place, then shift to the alpha release with the current repository head, and lots of warnings about how many things could change (particularly with the 'image handlers' like the gdcm code, and the deferred read vs memmap issues). I would really have liked to work those things out more fully, but I realize now it had been stopping me with just getting on with things. After the basic structure of old dicom and new pydicom are in place, we can tackle the remaining big issues one by one.

@SimonBiggs
Copy link
Contributor

SimonBiggs commented Jan 19, 2017 via email

@SimonBiggs
Copy link
Contributor

SimonBiggs commented Jan 19, 2017 via email

@darcymason
Copy link
Member

@SimonBiggs I had originally hoped that what you are suggesting could be done, just to have both installed, basically. So if someone does pip install pydicom, they get the new one and the old one. However, I'm wondering how this works if there is an existing older version of dicom already locally installed. Some code may require a particular older version of dicom and we have to be careful about overwriting it.

The from pydicom import * won't work as there are more backwards-incompatible changes than just the overall library name. That's if I've understood correctly that the idea was to make the old dicom kind of redirect to the new pydicom code.

@SimonBiggs
Copy link
Contributor

Make sense. I guess just need to bite the bullet and make the breaking change. I think the 1.0.0 is the perfect time for it. People will understand, it's kind of implied when a 0.* version is installed that the way the package works will probably change a bit before 1.0.0.

@stevenengler
Copy link

I agree, I think packaging the old version and new version together in version 1.0.0 with pip install pydicom could get confusing, plus then you'd have to keep supporting the old version for the foreseeable future in other releases like 1.0.1, 1.0.2, etc. Ideally people should be using specific package versions in their requirements, such as pydicom >=0.9.9, <1.0.0. No one should be upset if a 1.0.0 release breaks compatibility, but if you create a new package named 'dicom' and leave 'pydicom' as 0.9.9 forever (or maybe one more update with a deprecation warning), that's also reasonable I think. Just my 2 cents. Anyways looking forward to a release that supports python 3!

@darcymason
Copy link
Member

darcymason commented Jan 24, 2017

Update: created branch dicom from v0.9.9 release, added a warning on import, and a section for readthedocs user guide discussing the transition. Comments welcome.

Edit: the dicom branch is what we would be posted to package dicom on PyPI. The rst doc additions would also be ported to the master branch and be in the pydicom 1.0 release.

@darcymason
Copy link
Member

So dicom has been registered with PyPi as its own package, as version 0.9.9.post1, with a warning that it is no longer maintained, and a pointer to readthedocs to get more info.

Strangely, the wheel I built for python2.7 when installed back in a virtualenv, on import dicom gave an OverflowError for Tag, as if there was a 32/64 bit mismatch. So instead I went with the source distribution for py2. When that is pip installed (which converts to wheel as part of the process) it works fine, at least in my Windows environment. If anyone knows what is going on there, I'd be very curious to understand it.

@cancan101
Copy link
Contributor Author

How did you build the wheel? And can you post a stack trace?

@cancan101
Copy link
Contributor Author

Also, what was the name of the wheel before you removed it?

@darcymason
Copy link
Member

Here is the process including the stack trace. The pydicom-clean directory is just a git clone of my local repository to ensure no extra files that were not in version control. The python version at c:\python27\python.exe is the same used to create the wheel, simply using c:\python27\python setup.py bdist_wheel. It creates the name shown in the text below -- the py2-none-any.

C:\Users\Darcy\Envs>mkvirtualenv -p c:\python27\python.exe dic2
Running virtualenv with interpreter c:\python27\python.exe
New python executable in C:\Users\Darcy\Envs\dic2\Scripts\python.exe
Installing setuptools, pip, wheel...done.

(dic2) C:\Users\Darcy\Envs>pip install c:\git\pydicom-clean\source\dist\dicom-0.
9.9.post1-py2-none-any.whl
Processing c:\git\pydicom-clean\source\dist\dicom-0.9.9.post1-py2-none-any.whl
Installing collected packages: dicom
Successfully installed dicom-0.9.9.post1

(dic2) C:\Users\Darcy\Envs>python
Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec  5 2015, 20:40:30) [MSC v.1500 64 bit (
AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import dicom
C:\Users\Darcy\Envs\dic2\lib\site-packages\dicom\__init__.py:53: UserWarning:
This code is using an older version of pydicom, which is no longer
maintained as of Jan 2017.  You can access the new pydicom features and API
by installing `pydicom` from PyPI.
See 'Transitioning to pydicom 1.x' section at pydicom.readthedocs.org
for more information.

  warnings.warn(msg)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\Darcy\Envs\dic2\lib\site-packages\dicom\__init__.py", line 80,
in <module>
    from dicom.filereader import read_file, read_dicomdir  # noQA
  File "C:\Users\Darcy\Envs\dic2\lib\site-packages\dicom\filereader.py", line 14
, in <module>
    from dicom.tag import TupleTag
  File "C:\Users\Darcy\Envs\dic2\lib\site-packages\dicom\tag.py", line 110, in <
module>
    ItemTag = TupleTag((0xFFFE, 0xE000))  # start of Sequence Item
  File "C:\Users\Darcy\Envs\dic2\lib\site-packages\dicom\tag.py", line 106, in T
upleTag
    return BaseTag(long_value)
OverflowError: Python int too large to convert to C long

@cancan101
Copy link
Contributor Author

Try creating a setup.cfg file and putting in it:

[wheel]
universal=1

and then rebuilding

@darcymason
Copy link
Member

I thought I couldn't do that as we have 2to3? My memory is vague on this now as later it was converted to universal code that worked for both py2 and py3, but at least in the old release the 'use_2to3' is True in setup.py.

@cancan101
Copy link
Contributor Author

I see that error you are having is a known issue on windows, though not sure why wheel vs source would matter.

@ZeroCool2u
Copy link

If it's really just on Windows, could a Linux VM be spun up quick just to push past the error?

@stevenengler
Copy link

stevenengler commented Jan 27, 2017

@ZeroCool2u The Windows wheel needs to be built on a Windows computer, so wouldn't help in this case. (Maybe it's possible for pure-Python packages, I've never looked, but that would seem unsafe)

@darcymason Building the wheel, pip installing it, and importing dicom works fine on my Win 10 + Python 2.7 64-bit (Miniconda) environment. Maybe try activating your dic2 environment before you build your wheel instead of calling the python27 executable directly to make sure it's using the correct wheel package? But I can't reproduce the problem, so this is a guess.

@darcymason
Copy link
Member

@stevenengler, building the wheel from within the env sounded great, but doesn't seem to work for me - still the same error message.

However, as strange as this all is, I seem to have no problem installing from pypi via pip using the source archive for python 2. Does anyone know if not having the wheel actually limits anything?

@ZeroCool2u
Copy link

@darcymason I'm not sure about having the wheel, but from the stack trace it looks like the issue is a unsuccessful type conversion when Python tries to use a C module. It seems like in Tag.py the return value is cast as a long, but the cast isn't done in a way that the C module detects it. Maybe init the variable to 0 early on and explicitly define it's type then? Not sure if that would help, but that's what the stack trace looks like it's saying.

@stevenengler
Copy link

@darcymason I think installing from source shouldn't be a problem for anyone. Probably the biggest advantage of having a wheel is that it includes any compiled c/c++ code (extra important on Windows), but for pydicom there isn't any, so I think having just the source is fine. Another advantage to having a wheel is that the installation is quicker, but that's only minor. In general it's good to support a wheel, but in this case where there is some strange bug, I think it's fine to just include the source and you won't limit anything. Plus this is still considered the old version, and you can always add the wheel later if you need to.

@fedjo
Copy link

fedjo commented Jun 6, 2017

Any news about the 1.0.0 release?

Is installing from git stable?

Thanks

@darcymason
Copy link
Member

The master should generally be stable at this point. One possible exception is the format of images returned from decompression (gdcm, jpeg_ls), as in #273 and related issues. I'd like to see that solved before the 1.0.0 release.

@darcymason
Copy link
Member

darcymason commented Feb 11, 2018

Closing this issue, with discussion about release moved to #523.

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

No branches or pull requests

9 participants