Skip to content

Comments

Updating Friend Production#75

Draft
nshadskiy wants to merge 3 commits intomainfrom
update_friend_processing
Draft

Updating Friend Production#75
nshadskiy wants to merge 3 commits intomainfrom
update_friend_processing

Conversation

@nshadskiy
Copy link
Contributor

No description provided.

@nshadskiy nshadskiy changed the title Updateing Friend Production Updating Friend Production Jan 28, 2026
@tvoigtlaender
Copy link
Contributor

tvoigtlaender commented Jan 28, 2026

Hi,

I believe it would make more sense to instead change the underlying function self.local_path() function for the friends classes where you essentially rely on two production tags (the production_tag and the friend_name).
Something like:

    def local_path(self, *path):
        return os.path.join(
            (
                self.local_output_path
                if self.is_local_output
                else os.getenv("ANALYSIS_DATA_PATH")
            ),
            self.production_tag,
            self.friend_name,
            self.__class__.__name__,
            *path,
        )

and

    def remote_path(self, *path):
        parts = (self.production_tag,) + (self.friend_name,) + (self.__class__.__name__,) + path
        return os.path.join(*parts)

in the relevant friend classes.

@tvoigtlaender
Copy link
Contributor

Also, the nick should already be in the file structure (inside the htcondor_files). Are you sure you want to have it in there twice?

@nshadskiy
Copy link
Contributor Author

nshadskiy commented Jan 28, 2026

I only want this behavior for the htcondor files, not for other use cases. Therefore:

  • To your first comment: It's not that easy, you have to differentiate between CROWNRun and CROWNFriends production which is defined by the self.__class__.__name__ I belief. So you cannot put it before that. What we actually want to change is *path . A change in remote_path is not needed because we do not write the htcondor files there.
  • To your second comment: The nick is the sample name and I don't see where it should be included twice, at least in my test it looked fine.

@tvoigtlaender
Copy link
Contributor

tvoigtlaender commented Jan 28, 2026

As to the first point, what stops you from overwriting the function in the CROWNFriends and CROWNMultiFriends classes only?
At the end of the day, this is a more fundamental issue than just the location of the HTC files as all files from the Friend classes with an identical production_tag, but different friend_name end up with the same local/remote_path.
You can fix this with your approach, but the underlying issue is not resolved this way.
The benefit of removing the underlying issue would be that the naming conventions in the tasks could be cleaned up a bit. It's my personal opinion, but right now there is no clear system when things land in separate directories and when they just receive a file name with all information joined via _'s.

As to the second point, maybe what I saw was only in relation to the MultiFriends class. If you tested that and it wasn't present before, then the nick in the file name makes sense.

@a-monsch
Copy link
Contributor

As to the first point, what stops you from overwriting the function in the CROWNFriends and CROWNMultiFriends classes only? At the end of the day, this is a more fundamental issue than just the location of the HTC files as all files from the Friend classes with an identical production_tag, but different friend_name end up with the same local/remote_path. You can fix this with your approach, but the underlying issue is not resolved this way. The benefit of removing the underlying issue would be that the naming conventions in the tasks could be cleaned up a bit. It's my personal opinion, but right now there is no clear system when things land in separate directories and when they just receive a file name with all information joined via _'s.

On a side note here: In any case, the main goal to have a naming that in all cases is distinct should be the main goal. If this is achievable by building something future-robust, then this would be beneficial for us, @nshadskiy?

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.

3 participants