chianti-atomic / ChiantiPy

ChiantiPy is a python package to calculate the radiative properties of astrophysical plasmas based on the CHIANTI atomic database
63 stars 32 forks source link

build failure #67

Closed kdere closed 7 years ago

kdere commented 7 years ago

that PR did not build competely

install worked for Python 2.7, 3.4 but not 3.5

none of the tests were built

sphinx-build worked OK for 2.7, 3.4 and 3.5

I run on 3.4.5

wtbarnes commented 7 years ago

A few of them appear to be from the CHIANTI dbase zipfile not being transferred so no big deal there.

The last two are failures result from the changed abundanceName keyword from #66 . I'll update those testsradLoss.

Actually its that the continuum class is used in radLoss with the abundanceName keyword. Yay for tests!

wtbarnes commented 7 years ago

Travis CI creates a build for each PR. I would recommend waiting to merge each PR until all tests have passed. That way any bugs can be fixed before they are merged into the main codebase.

kdere commented 7 years ago

You are right about that. I have been a little too quick to just do the PR

wtbarnes commented 7 years ago

This can be closed. Fixed by #68

kdere commented 7 years ago

one final comment. The CHIANTI database is also available for download file-by-file. Perhaps this would be better than downloading the whole tar file. SSWIDL get these files from:

kdere commented 7 years ago

the previous comnet was suposed to include http://chiantidatabase.org/sswidl/dbase

wtbarnes commented 7 years ago

Maybe so. On RTD and Travis I had just downloaded the whole database because I wasn't sure which files were being read at import.

I think the reason the download phase in the build might be a bottleneck is because four workers are all trying to download the tar file at the same time. Is it possible that this kind of traffic is overwhelming the CHIANTI server? I'm assuming its not used to sudden bursts of traffic like that.

This can be solved by just limiting the number of concurrent jobs we run on Travis.

kdere commented 7 years ago

Some times the server keeps up with the traffic and sometimes not. This weekend was OK.

I think that tools/util/data is what reads the necessary data. These include the Ionization potential file, the ionization equlibrium files,the abundance files, and the masterlist

if things get too difficult, we could try limiting the build to these. However, I have learned how to restart the various steps in the build, one-by-one and this gets by that problem.

Also, I send you an invitation to join the team. I was not too clear on what that meant but what is was wondering, is whether you @wtbarnes wanted to be able to merge PR requests on your own?

wtbarnes commented 7 years ago

Yes I'd be happy to help merge in some PRs. I think if you grant me permission to merge PRs, this should also let me restart RTD and Travis builds as well when failures come up. Hopefully this takes some of the burden off of you!

I'm happy to merge PRs (either from myself or from others), but I think its important to still have your approval on these (in the form of a 👍 or +1 in the comments to the PR ) before they get merged into the main codebase. That way there is agreement on changes to the codebase before it is merged. The SunPy folks have outlined a procedure for reviewing PRs. It might not be a bad idea to use a system similar to this one.

kdere commented 7 years ago

It seems you are now a member. It seems like I need to make you an owner to do PRs. That is fine with me. The point of getting my approval sounds good. We need to find some way to make sure that we are not working on the same pieces of SW. Right now it is just in the form of comments here an there. The SunPy procedure is a bit much for really a 2 active member team.

wtbarnes commented 7 years ago

I believe you can create groups in order to customize permissions ? I'm not overly familiar with doing this on GitHub, but I think by creating a group (e.g. "Maintainers" or something), you can grant PR access without having full ownership over the repository?

Yes I agree the SunPy procedure is a bit over the top for this repo at the moment. Perhaps the minimum requirements for merging should be:

  1. A +1 or 👍 from you (@kdere)
  2. All tests passing on Travis CI
wtbarnes commented 7 years ago

I think the best way to ensure we aren't working on the same pieces of code is to open a PR every time you begin working on a feature. As you make commits to your fork while you work on the feature, these will show up in the PR.

When someone is ready for the PR to be merged, they can tag you with a review request (e.g. @kdere please review) and if you're happy with it and the tests have passed, then merge. That way, all work is done out in the open and it is obvious to everyone what features/pieces of code are being worked on.

kdere commented 7 years ago

Sounds good. I did not realize that you could open a PR unless you were ready to actually do it. I will give it a try.

On 12/05/2016 01:24 PM, Will Barnes wrote:

I think the best way to ensure we aren't working on the same pieces of code is to open a PR every time you begin working on a feature. As you make commits to your fork while you work on the feature, these will show up in the PR.

When someone is ready for the PR to be merged, they can tag you with a review request (e.g. @kdere https://github.com/kdere please review) and if you're happy with it and the tests have passed, then merge. That way, all work is done out in the open and it is obvious to everyone what features/pieces of code are being worked on.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/chianti-atomic/ChiantiPy/issues/67#issuecomment-264934108, or mute the thread https://github.com/notifications/unsubscribe-auth/AAtIViOGZG_oH-Qr0QAAr9f0vdkB3NyNks5rFFb4gaJpZM4LC95D.

-- Kenneth P. Dere Research Professor of Solar Physics Department of Physics and Astronomy George Mason University kdere@gmu.edu

kdere commented 7 years ago

I will look into making a group, maybe maintainers.

wtbarnes commented 7 years ago

This issue can also be closed. Looks like the original problem was resolved a while back.