Skip to content

Conversation

@Aggarwal-Raghav
Copy link
Contributor

@Aggarwal-Raghav Aggarwal-Raghav commented Nov 21, 2025

BEFORE FIX:
swimlane_amparser_error
coutner_diff_error

AFTER FIX:
swimlane-working
Counter_diff_fix

with open(dag_json_file1) as data_file:
file1_dag_json = json.load(data_file)["dag"] if file1_using_dag_json else json.load(data_file)
counters = file1_dag_json['otherinfo']['counters']
for group in counters['counterGroups']:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added safe check because of such errors
Image

@abstractdog abstractdog self-requested a review November 21, 2025 08:54
@abstractdog
Copy link
Contributor

this is awesome @Aggarwal-Raghav , thanks! +1

@Aggarwal-Raghav
Copy link
Contributor Author

The goal was to keep the changes at minimum and convert to python3. Otherwise there is scope of improvement in scripts by using argparser instead of sys.argv, pathlib instead of os.path.

@abstractdog
Copy link
Contributor

The goal was to keep the changes at minimum and convert to python3. Otherwise there is scope of improvement in scripts by using argparser instead of sys.argv, pathlib instead of os.path.

yeah, small refactors are always welcome, absolutely fit this PR

@tez-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 31m 27s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+0 🆗 shelldocs 0m 0s Shelldocs was not available.
+1 💚 @author 0m 0s The patch does not contain any @author tags.
_ master Compile Tests _
-1 ❌ pylint 0m 1s /branch-pylint-stderr.txt Error running pylint. Please check pylint stderr files.
_ Patch Compile Tests _
+1 💚 codespell 0m 4s No new issues.
-1 ❌ blanks 0m 0s /blanks-eol.txt The patch has 7 line(s) that end in blanks. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
-1 ❌ blanks 0m 0s /blanks-tabs.txt The patch 84 line(s) with tabs.
+1 💚 markdownlint 0m 2s The patch generated 0 new + 45 unchanged - 1 fixed = 45 total (was 46)
-1 ❌ pylint 0m 2s /patch-pylint-stderr.txt Error running pylint. Please check pylint stderr files.
+1 💚 pylint 0m 2s No new issues.
+1 💚 shellcheck 0m 0s No new issues.
_ Other Tests _
-1 ❌ asflicense 0m 51s /results-asflicense.txt The patch generated 1 ASF License warnings.
33m 24s
Subsystem Report/Notes
Docker ClientAPI=1.52 ServerAPI=1.52 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-444/1/artifact/out/Dockerfile
GITHUB PR #444
Optional Tests dupname asflicense codespell detsecrets pylint markdownlint shellcheck shelldocs
uname Linux d201e749b075 5.15.0-153-generic #163-Ubuntu SMP Thu Aug 7 16:37:18 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality /home/jenkins/jenkins-home/workspace/tez-multibranch_PR-444/src/.yetus/personality.sh
git revision master / 92119cb
Max. process+thread count 52 (vs. ulimit of 5500)
modules C: tez-tools U: tez-tools
Console output https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-444/1/console
versions git=2.43.0 maven=3.8.7 codespell=2.0.0 markdownlint=0.23.2 shellcheck=0.7.1
Powered by Apache Yetus 0.15.1 https://yetus.apache.org

This message was automatically generated.

@maheshrajus
Copy link
Contributor

LGTM
@Aggarwal-Raghav can you please check the above failures in yetus report. thanks !

@Aggarwal-Raghav
Copy link
Contributor Author

LGTM @Aggarwal-Raghav can you please check the above failures in yetus report. thanks !

thanks for the review @maheshrajus , sure, I'll fix the failures.

@Aggarwal-Raghav
Copy link
Contributor Author

@maheshrajus @abstractdog , it seems that the pylint is not working in yetus based on /branch-pylint-stderr.txt ,
As i ran the pylint in local, there are lot of issues unrelated to the change. I can surely check/fix them along with shellcheck, blanks, asfheader etc.

Also, 1 major flaw in python scripts are, tabs are not converted to spaces and has un-even indentation.

Python's official style guide, PEP 8, strongly recommends using spaces for indentation, specifically four consecutive spaces per indentation level. While Python technically allows both tabs and spaces for indentation, and even disallows mixing them within the same file, spaces are the widely preferred and recommended convention

Scripts are old and requires some decent changes. Personally, I'm ready and committed to fix them. Will also accomodate that pathlib and argparser changes as well. I'll use both https://github.com/astral-sh/ruff and pylint to ensure scripts are of good quality.

Screenshot 2025-11-21 at 3 28 10 PM

@abstractdog
Copy link
Contributor

@maheshrajus @abstractdog , it seems that the pylint is not working in yetus based on /branch-pylint-stderr.txt , As i ran the pylint in local, there are lot of issues unrelated to the change. I can surely check/fix them along with shellcheck, blanks, asfheader etc.

Also, 1 major flaw in python scripts are, tabs are not converted to spaces and has un-even indentation.

Python's official style guide, PEP 8, strongly recommends using spaces for indentation, specifically four consecutive spaces per indentation level. While Python technically allows both tabs and spaces for indentation, and even disallows mixing them within the same file, spaces are the widely preferred and recommended convention

Scripts are old and requires some decent changes. Personally, I'm ready and committed to fix them. Will also accomodate that pathlib and argparser changes as well. I'll use both https://github.com/astral-sh/ruff and pylint to ensure scripts are of good quality.

Screenshot 2025-11-21 at 3 28 10 PM

thanks a lot for taking a look at this!
it's fine to fix all these things in follow-up tickets, mainly because they tend to fall into different categories:

  1. python problems with our code (a lot)
  2. pylint problems that needs yetus configuration maybe

ruff format
@Aggarwal-Raghav
Copy link
Contributor Author

Aggarwal-Raghav commented Nov 21, 2025

i have removed the tabs with space (just indentation change). that should fix the /blanks-eol.txt /blanks-tabs.txt and requirements.txt for now

@tez-yetus
Copy link

💔 -1 overall

Vote Subsystem Runtime Logfile Comment
+0 🆗 reexec 0m 27s Docker mode activated.
_ Prechecks _
+1 💚 dupname 0m 0s No case conflicting files found.
+0 🆗 detsecrets 0m 0s detect-secrets was not available.
+0 🆗 shelldocs 0m 0s Shelldocs was not available.
+1 💚 @author 0m 1s The patch does not contain any @author tags.
_ master Compile Tests _
-1 ❌ pylint 0m 1s /branch-pylint-stderr.txt Error running pylint. Please check pylint stderr files.
_ Patch Compile Tests _
+1 💚 codespell 0m 4s No new issues.
+1 💚 blanks 0m 0s The patch has no blanks issues.
+1 💚 markdownlint 0m 3s The patch generated 0 new + 45 unchanged - 1 fixed = 45 total (was 46)
-1 ❌ pylint 0m 2s /patch-pylint-stderr.txt Error running pylint. Please check pylint stderr files.
+1 💚 pylint 0m 3s No new issues.
+1 💚 shellcheck 0m 0s The patch generated 0 new + 0 unchanged - 1 fixed = 0 total (was 1)
_ Other Tests _
-1 ❌ asflicense 0m 49s /results-asflicense.txt The patch generated 1 ASF License warnings.
2m 21s
Subsystem Report/Notes
Docker ClientAPI=1.52 ServerAPI=1.52 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-444/2/artifact/out/Dockerfile
GITHUB PR #444
Optional Tests dupname asflicense codespell detsecrets pylint markdownlint shellcheck shelldocs
uname Linux 875f66e6fb1c 5.15.0-161-generic #171-Ubuntu SMP Sat Oct 11 08:17:01 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Build tool maven
Personality /home/jenkins/jenkins-home/workspace/tez-multibranch_PR-444/src/.yetus/personality.sh
git revision master / d317d55
Max. process+thread count 51 (vs. ulimit of 5500)
modules C: tez-tools U: tez-tools
Console output https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-444/2/console
versions git=2.43.0 maven=3.8.7 codespell=2.0.0 markdownlint=0.23.2 shellcheck=0.7.1
Powered by Apache Yetus 0.15.1 https://yetus.apache.org

This message was automatically generated.

@Aggarwal-Raghav
Copy link
Contributor Author

Earlier pylint rating:

logsplit.py: Your code has been rated at 6.85/10
swimlane.py: Your code has been rated at 0.00/10
counter-diff.py: Your code has been rated at 0.00/10
amlogparser.py: Your code has been rated at 0.00/10

pylint rating after reformatting:

logsplit.py: Your code has been rated at 6.99/10
swimlane.py: Your code has been rated at 6.17/10
counter-diff.py: Your code has been rated at 8.35/10
amlogparser.py: Your code has been rated at 6.71/10

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.

4 participants