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 molpx recipe #741

Merged
merged 10 commits into from
Mar 16, 2017
Merged

added molpx recipe #741

merged 10 commits into from
Mar 16, 2017

Conversation

marscher
Copy link
Contributor

No description provided.

@marscher
Copy link
Contributor Author

cc @gph82

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

I'm pretty sure the noarch_python will fail. There is a bug involving how the file name string is formed such that it looses its python version identity as part of the string, and I can't figure out why yet.

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

Fail is not the right word, it might build, but there will be no way to tell in the file name what python version is was built for

@marscher
Copy link
Contributor Author

Can you please point me to the lines of code, which are failing?

@marscher
Copy link
Contributor Author

marscher commented Mar 15, 2017 via email

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

Sure. For context though, this is all based on what I think the code is suppose to be doing.

We modify the Python version and the Numpy version of the package here to ensure that Py/NPy versions we are building against are the targeted once, and not those of the system default (e.g. building Python 2.7 version when system is 3.6). There was a change to the conda-build code which changed the behavior of that block which I managed to fix in this PR.

As I understand, the noarch_python flag has been broken for a while, before I started helping with this code, based on all the commented out noarch_python lines in the meta.yaml files. I don't know what was broken so I am still not sure what the proper fix is. You can see in the Travis log down at line 3057 (if that does not directly take you there) that it tries 4 times to build a package called molpx-0.1.3-py_0.tar.bz2 where the missing information should be pyXX_0.tar.bz2. The 0 is just the build number.

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

If you have an idea on how to fix it, I'm all for trying it!

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

I am guessing its the get_output_file_path(m, cfg). Would we need to change that to get_output_file_paths(m, config=config)?

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

And you are about 5 min ahead of me I see

@marscher
Copy link
Contributor Author

marscher commented Mar 15, 2017 via email

@marscher
Copy link
Contributor Author

marscher commented Mar 15, 2017 via email

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

Hmm... Doing some local tests it looks like its still going to mess up the file path. We'll see if it shows up in the Travis build

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

It also looks like it is going to mess up the file path, even if we dont do anything to the config file. I think I might have an answer. I'm going to do some local test

@marscher
Copy link
Contributor Author

marscher commented Mar 15, 2017 via email

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

Shouldn't it also have a python version name attached to it so we now that the builds were compiled against python 2 and all the python 3?

@marscher
Copy link
Contributor Author

marscher commented Mar 15, 2017 via email

@marscher
Copy link
Contributor Author

marscher commented Mar 15, 2017 via email

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

The mission critical point about noarch_python is also that it is invariant against the python major version

I was wondering if that were the case, but I have my doubts as attempting to go up to Python 3.6 caused many, many packages to break here. So I would only accept that logic if the package did not need python at all, which is where I get concerned that we would be skipping tests on all versions of python to ensure code stability.

As I understand from this issue, the linking happens at test time so I can see how that might create the folder there. Check the final output file name that is proposed for the upload and see if it says win-32 or noarch

@marscher
Copy link
Contributor Author

marscher commented Mar 15, 2017 via email

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

I am actually not seeing this problem on my local testing. Setting noarch_python correctly gives the noarch directory as the output, even after doing the parse_again(). I am however, far more concerned about packages which specify the noarch_python then requires numpy x.x which I am not sure are going to be tested correctly. Are you sure it does not just assign an architecture for the test since it kind of has to?

I am using 'auograd' as my test since it has both python and numpy requirements, but also had its noarch_python line commented out.

@marscher
Copy link
Contributor Author

marscher commented Mar 15, 2017 via email

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

If it depends also on NumPy. The x.x setting should actually be used to require a certain ABI version.

The package I am using as test does have the x.x requirement on Numpy. You are saying that it should not matter for the builds here, but the end-user will need to specify a numpy version at test/run install time (which they would have to do anyways). I'm also not sure what the ABI acronym is. Sorry if these are somewhat naive questions, I'm relatively new to package building and distribution.

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

I also know right now the conda-build-all script cannot handle noarch packages which also require numpy x.x since it will try to build that package and fail. The script in general would need to be modified to handle that case. Of course there is talk if we should even be trying to maintain our own conda-build-all script for this exact type of problem (a la #735).

@rmcgibbo
Copy link
Contributor

rmcgibbo commented Mar 15, 2017 via email

@Lnaden
Copy link
Contributor

Lnaden commented Mar 15, 2017

noarch and numpy x.x don't make sense together

Okay, that clears a number of things up, it just means that the autograd package meta.yaml file is flawed. Thanks!

@rmcgibbo
Copy link
Contributor

rmcgibbo commented Mar 15, 2017 via email

@rmcgibbo
Copy link
Contributor

rmcgibbo commented Mar 15, 2017 via email

@marscher marscher merged commit 25cf98c into omnia-md:master Mar 16, 2017
@marscher marscher deleted the add_molpx branch March 16, 2017 07:46
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.

None yet

3 participants