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

Clean up build of input index file #234

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

Clean up build of input index file #234

wants to merge 3 commits into from

Conversation

pdeiml
Copy link
Collaborator

@pdeiml pdeiml commented Apr 13, 2018

This PR is on top of #211. Hence, you should firstly review and merge #211 before looking at the diff of this PR!

The plan is to have to classes InputCollection and OutputCollection each handling - as the names suggests - every data in the input and output folders, respectively.

This PR starts to write the InputCollection and it implements to first things:

  1. A list of all info.yaml files (stored in input folder) is created via the class InputInfoCollection (see inline comment).
  2. All of these info-files are validated (see inline comment).
  3. Secondly, the input_index_file is created after (!) the info-files are validated (see inline comment).
    Step 2) and 3) are handled via a run function in InputCollection (see inline comment).

In the future these three steps should be the very first thing make.py does! This PR does not (!) change the procedure of make.py, it only introduces a ugly way to run the upper steps when make.py collection --step input-index is executed (see inline comment). There needs to be done a follow up PR which cleans up make.py.

The second small commit is only a small fix in the _make_index_file_input function so that it uses the to_list() function of InputInfoCollection. But the functionality is not changed.

@pdeiml pdeiml added the coding label Apr 13, 2018
def __init__ (self, config):
self.config = config
self.data = InputData.read()
self.info_files = InputInfoCollection.read()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line creates a list of all info files which are present in the input folder (step 1 in comment).

self.info_files = InputInfoCollection.read()

def run(self):
self._validate_info_files()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here, all info files which are present in the input folder are validated (step 2 in comment).


def run(self):
self._validate_info_files()
self._make_index_file_for_input()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line creates the input index file and stores it (step 3 in comment).

resource.location = str(info_filename.parent.relative_to(self.config.in_path) / dataset)
resources.append(resource)

self.index = GammaCatResourceIndex(resources).sort()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure whether this definition of the class instance variable self.index is pythonic or not. Currently, self.index is not used anywhere but I think in the future we will handle a lot of things via this index, hence, it is nice to have it as a class instance variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not used from elsewhere at the moment, better to keep it as a local variable only.

pdeiml added 2 commits April 13, 2018 10:18
Use InputCollection class in make.py
Use InputInfoCollection to build index file
@@ -212,8 +251,6 @@ def run(self):
step = self.config.step
if step == 'all':
self.process_all()
elif step == 'input-index':
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not working anymore because it is not handled in this class.

@@ -233,7 +270,6 @@ def input_data(self):
return InputData.read()

def process_all(self):
self.make_index_file_for_input()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above. Not part of this class anymore.

@@ -45,7 +45,10 @@ def cli_collection(step):
out_path=gammacat_info.out_path,
step=step,
)
CollectionMaker(config).run()
if (step == 'input-index'):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know, this looks ugly and need follow up change! But it works, hence, I would leave it as it is for now and change it later.

@pdeiml
Copy link
Collaborator Author

pdeiml commented Apr 13, 2018

I don't understand the error of travis ci. It is a http error?
make.py all does work locally, hence, there shouldn't be an error.

Copy link
Contributor

@cdeil cdeil left a comment

Choose a reason for hiding this comment

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

@pdeiml -I merged #211 .

Should I merge this now, or are some edits neede first?

(I don't have time to review, I trust the change is a good one.)

resource.location = str(info_filename.parent.relative_to(self.config.in_path) / dataset)
resources.append(resource)

self.index = GammaCatResourceIndex(resources).sort()
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not used from elsewhere at the moment, better to keep it as a local variable only.

@pdeiml
Copy link
Collaborator Author

pdeiml commented Jul 9, 2018

Honestly, I don't know whether you can merge this now after merging #211 :-D
I won't have time the next days but I will try to take a look to this again and then I give you feedback here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants